Bug 5192 - ThinLinc commands should not use DISPLAY to identify ThinLinc session
Summary: ThinLinc commands should not use DISPLAY to identify ThinLinc session
Status: CLOSED FIXED
Alias: None
Product: ThinLinc
Classification: Unclassified
Component: Misc (show other bugs)
Version: 4.2.0
Hardware: PC Unknown
: P2 Normal
Target Milestone: 4.3.0
Assignee: Peter Åstrand
URL:
Keywords: ossman_tester, prosaic
: 3797 (view as bug list)
Depends on:
Blocks:
 
Reported: 2014-06-18 13:22 CEST by Peter Åstrand
Modified: 2014-10-29 16:48 CET (History)
2 users (show)

See Also:
Acceptance Criteria:

Attachments
Add an attachment (proposed patch, testcase, etc.)

Description Peter Åstrand cendio 2014-06-18 13:22:58 CEST 
Some ThinLinc commands and utilities looks at the DISPLAY environment variable in order to identify the ThinLinc session. prefix.py:get_tl_session_dir()is doing this as a fallback in case TLSESSIONDATA is missing. Others, such as ctccommon.py:get_vnc_port() is using this to determine the RFB port number. 

Unfortunately, the DISPLAY environment is not reliable for this. It should be treated just for what it is: determining which Xserver to communicate with. Although VSM Agent sets up a DISPLAY which is equal to the TL session number, users are free to modify DISPLAY later. For example, assume that the users runs Xnest, Xvnc or a similar software inside the session. DISPLAY could then be redirected to that Xserver, but the user still wants to be able to use utilities such as tl-mount-localdrives. Another case is of course when DISPLAY is set to another physical workstation or server. 

Another reason for fixing this bug is that we have parsing code for DISPLAY in several locations, often with bugs (see bug 3797). 

My suggestion is that for things that needs to use TLSESSIONDATA, I think that TLSESSIONDATA should simply be a requirement, and no DISPLAY fallback should be used. For get_vnc_port() etc, we have at least two options:

1) (again) use and require TLSESSIONDATA, which ends with the session number. 

2) introduce a new variable called TLSESSIONNUM or similar.
Comment 1 Pierre Ossman cendio 2014-06-24 10:32:13 CEST 
Another solution:

3) Put a file in $TLSESSIONDATA that specifies the display number. This avoids multiple required environment variables.
Comment 2 Peter Åstrand cendio 2014-07-09 10:39:12 CEST 
(In reply to comment #0)

> My suggestion is that for things that needs to use TLSESSIONDATA, I think that
> TLSESSIONDATA should simply be a requirement, and no DISPLAY fallback should be
> used.

Fixed in 29186.
Comment 3 Peter Åstrand cendio 2014-07-09 14:55:41 CEST 
*** Bug 3797 has been marked as a duplicate of this bug. ***
Comment 4 Peter Åstrand cendio 2014-07-09 15:40:51 CEST 
(In reply to comment #1)
> Another solution:
> 
> 3) Put a file in $TLSESSIONDATA that specifies the display number. This avoids
> multiple required environment variables.

Fixed in 29187.

The tester should make sure these tools still works:

tl-mount-localdrives
tl-umount-localdrives
tl-serial-redir
tl-notify
tl-run-rdesktop
tl-run-xstartup.d
tl-shadow-notify-helper
Comment 5 Henrik Andersson cendio 2014-09-19 10:13:42 CEST 
(In reply to comment #4)
> tl-umount-localdrives

Works as expected within and outside a valid ThinLinc Session

> tl-mount-localdrives

Asserts should be performed as early as possible to prevent doing stuff if it's not going to work. This is not the case in tl-mount-localdrives which produces duplicated error message "TLSESSIONDATA is not set. Are you i..." due to call to tl-umount-localdrives before checking if we are in a valid session.
Comment 6 Henrik Andersson cendio 2014-09-19 13:27:09 CEST 
(In reply to comment #4)
> tl-serial-redir
> tl-notify
> tl-run-rdesktop
> tl-run-xstartup.d
> tl-shadow-notify-helper

Each tool above works as expected within and outside a ThinLinc session.
Comment 7 Peter Åstrand cendio 2014-09-22 12:29:54 CEST 
(In reply to comment #5)
> (In reply to comment #4)
> > tl-umount-localdrives
> 
> Works as expected within and outside a valid ThinLinc Session
> 
> > tl-mount-localdrives
> 
> Asserts should be performed as early as possible to prevent doing stuff if it's
> not going to work. This is not the case in tl-mount-localdrives which produces
> duplicated error message "TLSESSIONDATA is not set. Are you i..." due to call
> to tl-umount-localdrives before checking if we are in a valid session.

Fixed in 29397.
Comment 8 Henrik Andersson cendio 2014-09-24 13:39:10 CEST 
(In reply to comment #7)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > tl-umount-localdrives
> > 
> > Works as expected within and outside a valid ThinLinc Session
> > 
> > > tl-mount-localdrives
> > 
> > Asserts should be performed as early as possible to prevent doing stuff if it's
> > not going to work. This is not the case in tl-mount-localdrives which produces
> > duplicated error message "TLSESSIONDATA is not set. Are you i..." due to call
> > to tl-umount-localdrives before checking if we are in a valid session.
> 
> Fixed in 29397.

Verified using build 4497. Works as expected.
Comment 9 Peter Åstrand cendio 2014-10-28 14:32:07 CET 
(In reply to comment #5)
> (In reply to comment #4)
> > tl-umount-localdrives
> 
> Works as expected within and outside a valid ThinLinc Session
> 
> > tl-mount-localdrives

Does not work for me:

# tl-umount-localdrives 
TLSESSIONDATA is not set. Are you in a proper ThinLinc session?
Comment 10 Peter Åstrand cendio 2014-10-28 14:48:25 CET 
(In reply to comment #9)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > tl-umount-localdrives
> > 
> > Works as expected within and outside a valid ThinLinc Session
> > 
> > > tl-mount-localdrives
> 
> Does not work for me:
> 
> # tl-umount-localdrives 
> TLSESSIONDATA is not set. Are you in a proper ThinLinc session?

r29544.
Comment 11 Pierre Ossman cendio 2014-10-29 13:33:59 CET 
tl-umount-localdrives -s still requires TLSESSIONDATA.
Comment 12 Peter Åstrand cendio 2014-10-29 13:36:59 CET 
(In reply to comment #11)
> tl-umount-localdrives -s still requires TLSESSIONDATA.

Another try in 29547.
Comment 13 Pierre Ossman cendio 2014-10-29 16:48:03 CET 
This version seems to work as advertised. :)
Note You need to log in before you can comment on or make changes to this bug.