Opened 7 years ago

Closed 7 years ago

#201 closed defect (fixed)

Incorrect check for started X server

Reported by: Alexey Loginov Owned by: Antoine Martin
Priority: major Milestone:
Component: Client Keywords:
Cc:

Description (last modified by Antoine Martin)

In trying to get a WinSwitch/xpra server on Ubuntu to work with a WinSwitch/xpra client on OSX, I had to fight with xmodmap a bit (need to use Alt for Eclipse, emacs, etc.). I found the check for running X server not to be quite right. The following diff is for an older version of winswitch/client/client_base.py (I can't use the latest due to https://xpra.org/trac/ticket/92), so the lines aren't current but their content hasn't changed.

The first change fixes the search of lines for "org.x.startx". In the old form, line.find returns -1 for any line without a match, triggering on the very first line ("PID Status Label"). Python documentation states a preference for "substring in string", when the position of substring in string is not needed. If you want to insist on "org.x.startx" at the very end of the line, the test could be: if line.endswith("org.x.startx"):

The second change corrects the check for a process that's running now ("0" in parts[1] appears to indicate that the process has terminated and returned 0). Checking that parts[0] is NOT "-" is a valid check for a running process, as well. The man pages are a bit vague.

The changes below work on my MacBookPros at home and at work with 10.5 and 10.6:

432c432
<                                       if line.find("org.x.startx$"):
---
>                                       if "org.x.startx" in line:
436c436
<                                                       started = parts[1]=="0"
---
>                                                       started = parts[1]=="-"

I hope this helps.
-Alexey

Change History (4)

comment:1 Changed 7 years ago by Antoine Martin

Description: modified (diff)
Owner: changed from Antoine Martin to Antoine Martin
Status: newaccepted

If the check for a terminated process is what you say it should be, shouldn't the new python code be:

    started = parts[1]!="-"

Instead of "==" ?

comment:2 in reply to:  1 Changed 7 years ago by Alexey Loginov

Replying to antoine:

If the check for a terminated process is what you say it should be, shouldn't the new python code be:

    started = parts[1]!="-"

Instead of "==" ?


No, the second column ([1]) is "-" for running processes. When the X server is started, launchctl list gives me something like:

5170	-	org.x.startx

When the X server is not started, I get:

-	0	org.x.startx

However, I found a more detailed man page that suggests that checking the first column ([0]) is best. From http://developer.apple.com/library/mac/#documentation/Darwin/Reference/ManPages/man1/launchctl.1.html:

  list [-x] [label]
              With no arguments, list all of the jobs loaded into launchd in three columns. The first column
              displays the PID of the job if it is running.  The second column displays the last exit status
              of the job. If the number in this column is negative, it represents the negative of the signal
              which killed the job.  Thus, "-15" would indicate that the job was terminated with SIGTERM.
              The third column is the job's label.
  ...

With all this, I'd suggest making just the following changes. We'd insist on ending with "org.x.startx" and check the first column ([0], guaranteed to be present, so no need for the length guard) for a numeric value (PID):

432c432
< 					if line.find("org.x.startx$"):
---
> 					if line.endswith("org.x.startx"):
435,436c435
< 						if len(parts)>=2:
< 							started = parts[1]=="0"
---
> 						started = parts[0].isdigit()

This works on my laptops (10.5 & 10.6).

Makes sense?
-Alexey

comment:3 in reply to:  description Changed 7 years ago by Alexey Loginov

FWIW, by #92 I meant the xpra #92 ('a' MIA), just to give it another bump. :-)
Thought I copied the full URL but I must not have: https://xpra.org/trac/ticket/92.

Last edited 7 years ago by Antoine Martin (previous) (diff)

comment:4 Changed 7 years ago by Antoine Martin

Description: modified (diff)
Resolution: fixed
Status: acceptedclosed

applied in r4902

(FYI: you had put the full URL to the xpra bug... which I had mistakenly changed thinking it was pointing to the same bug tracker as I was moving https URLs around.. now changed back)

Note: See TracTickets for help on using tickets.