GNOME.org
 

Re: [gdm-list] (PATCH) Re: gdm hung after killing child


Simon:


Thanks for responding.


Probably.  The patch is against 2.6 and not CVS head, but to get the
behavior that Simon wants he would have to do (TYPE_STATIC instead
of TYPE_LOCAL).


I see, yes, TYPE_LOCAL changed to TYPE_STATIC in HEAD.
I have found that this is because when a gdm slave is killed, it shuts down the session and exits with DISPLAY_ABORT, which means the master drops that display. 

Trying to kill the Xserver by killing the gdm slave process is not a
supported way of trying to kill the Xserver.  I think this patch
would overload the TERM/KILL handler in a way that is messy and
will cause the daemon to do unnecessary work when it is killed
normally via gdm-stop.
I don't see why it can't do both. If the daemon wants to kill its slaves, it should ignore whether they return with REMANAGE or not. It's the daemon's business how it handles that. 

I would entertain this, but the patch would likely need to be smarter.
For example, I notice in gdm.c that in mainloop_sig_callback it kills
the children on SIGINT/SIGTERM but doesn't set any flag to let the
gdm_cleanup_children function know to ignore any REMANAGE it receives.
This  would have to be working and well tested to consider accepting
doing it this way.


When the GDM daemon is killed via gdm-stop, it sends TERM signals to
any slaves that are running.  This is why the slaves abort their
sessions when they receive this signal.  No other process aside from
the daemon should be trying to send TERM/KILL signals to the slave
process.
The reason that the slave is targetted is because it is the parent of the session scripts and subsequently window/session-manager. In a situation of wanting to restart exactly one of several running local displays, it makes sense to kill the slave for your display (parent of the wm/startup scripts), rather than the whole daemon. 

I would think that there would be an interface you could use to kill
the session that would be a bit more clean that killing a DM process.
Aren't their interfaces for simply killing the xclient, such as sending
it a HUP?  The disadvantage of killing a DM process is that you
become tied to the way particular DM's work internally, which isn't
so good as you are noticing (different steps followed by different DM's
when using undocumented signals to make them do stuff).
The attached patch makes it exit with DISPLAY_REMANAGE for displays of type TYPE_LOCAL. Incidentally, this replicates xdm's behaviour when its slave is killed. 

How does xdm differentiate being killed by its parent because the user
really wants to kill GDM and the slave being killed otherwise.
xdm and gdm seem to operate oppositely at the moment. On a TERM, its slave will ultimately restart, yet on a HUP it stays dead (which I think is 
counter-expectations for a HUP).
I agree. I think it makes sense if GDM works the other way, with TERM/KILL causing the slave to die (as it currently does) and HUP 
causing it to restart.  That seems cleaner than the xdm behavior.
Also doing it this way avoids changing GDM's behavior for any users
who might be depending on the way TERM/KILL currently works.
If this is not acceptable (why?) then I have an alternative patch that handles the HUP signal and exits with REMANAGE when it receives one of them. 

I think enhancing the slave so it can handle a HUP signal to tell it
to restart itself is a better way of fixing this.
I think it is much more intuitive behaviour. Though I still think that losing the display "forever" after killing the slave with a TERM is counter-intuitive. Nevertheless, I'll settle for anything that stops me having to search process lists and just kill parent. 

I agree, although processes the the GDM slave shouldn't receive
TERM/KILL signals unless a sysadmin is running the command.  Plus it
isn't necessarily bad that GDM allows the slave to be shut down
completely.  There might be situations where that is useful.
On a side note, the slave's global "d" variable is initialised after the signal handlers are installed, so there is a race condition if a signal arrives before gdm_slave_run (so if one arrives, then blech). It's very unlikely to happen, but why not just d = display or d = 0 earlier?
Do you mean the "GSList displays" global that gets set in gdm_config_parse a few lines after setting the signal handlers? I think 
the "d" variable is grabbed out of this list, or am I missing something


No, I mean:

static GdmDisplay *d;

in slave.c


Thanks for pointing this race condition out.  I now set d=display in
gdm_slave_start, and continue to reset it in gdm_slave_run since
gdm_slave_run is called in a loop.  I made this fix in both the 2.8
and 2.13 CVS.

Brian
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]