Created attachment 2361 All the changes listed above I wanted to add X11 session support to mousepad, but ran into some gui4 session-client bugs and deficiencies, and had to hack it a bit. Bug fixes: client_session_new_full() sets the restart, clone etc. commands _by directly assigning to them_ instead of duplicating the arguments passed. So any call to set_whatever_command() or client_session_free() crashes when attempting to g_strfreev() the command(s), unless the **values passed to new_full() are allocated-and-not-freed by the caller. However, client_session_new() passes the same array for both restart and clone, and the array elements are taken directly from argv[], with obvious results. When client_session_new() generates the initial restart command, it does not strip the (previous) client id, if any, so set_clone_restart_commands() does not append the (newly given) client id. Of course, a when previous id exists, the new id will normally be the same - unless, for example, previous is invalid. If client_session_free() is called before session_init(), or after an unsuccessful session_init(), it calls disconnect(), which calls SmcCloseConnection() with session_connection = NULL and thus crashes. Improvements (well, hopefully): There was no way to cancel the shutdown from interact. For compatibility, I extended the session client structure with gboolean interact_2. The restart, clone etc. commands are set before invoking the application's save_yourself/save_phase_2 callbacks. SM spec says that interact and save_phase_2 should SetProperties, and the session client code does than, but without giving the application a change to set/alter them. And without that, for example, mousepad will have to set_restart_command() on each possible filename change (4 calls in callbacks.c and one more in main.c). So I moved the set_clone_restart_commands() invocation immediately before SmcSaveYourselfDone(). The --display (if missing) was appended only to the automatically generated client_session_new() restart/clone commands, and not to any commands set by the application. So I moved appending to set_clone_restart_commands(). client_session_new() initializes both clone and restart commands, so if the application sets a new restart command, it can not rely on the automatic restart -> clone copy. Altered client_session_new() to pass NULL, so the latest restart command is copied to clone. Added the missing client_session_set_interact_style() function. Currently, all SessionClient fields are accessed via functions, except for interact_style which must be directly assigned to. Optimizations: The largely identical code for setting the different commands in set_clone_restart_commands() is rebased a single function: set_property_from_command(client, command [x11 command name], ptr [command value], add_sm_id, add_display) The identical set_whatever_command()-s are now one-liners invoking the new copy_command() function. Notes: Using g_strdupv() instead of safe_strvdup() seems to work just fine. Not knowing why safe_strvdup() stands for, I left it as is. IMHO, SessionClient should only include a single session id variable, namely client_id, which should be equal to the previous id before init and to the new [given] id after; given_client_id should be a local variable of session_init(). But I didn't want to break compatibility. I tried to keep the code simple, since the session management is quite unpleasant by itself (to avoid a harsher expression). Last but not least, I compiled libxfcegui4.so and replaced my original shared library. No crashes, and the new library removed --sm-client-id from the clone commands of xfdesktop and xfwm. Well, that's about it. If you have any questions, or there are things you don't like, I'm open for discussion.
(In reply to comment #0) > Created an attachment (id=2361) [details] > All the changes listed above > > I wanted to add X11 session support to mousepad, but ran into some gui4 > session-client bugs and deficiencies, and had to hack it a bit. Yay! Our session client really sucks. Thanks for looking into this. > client_session_new_full() sets the restart, clone etc. commands _by directly > assigning to them_ instead of duplicating the arguments passed. So any call > to set_whatever_command() or client_session_free() crashes when attempting > to g_strfreev() the command(s), unless the **values passed to new_full() are > allocated-and-not-freed by the caller. However, client_session_new() passes > the same array for both restart and clone, and the array elements are taken > directly from argv[], with obvious results. Odd, I thought we'd already accepted a patch that fixes this, but I guess not. Must be floating around in bugzilla somewhere... > When client_session_new() generates the initial restart command, it does not > strip the (previous) client id, if any, so set_clone_restart_commands() does > not append the (newly given) client id. Of course, a when previous id > exists, the new id will normally be the same - unless, for example, previous > is invalid. Good... > If client_session_free() is called before session_init(), or after an > unsuccessful session_init(), it calls disconnect(), which calls > SmcCloseConnection() with session_connection = NULL and thus crashes. Yeah, I recall this got fixed in that same patch, but I guess no one checked it in. > There was no way to cancel the shutdown from interact. For compatibility, > I extended the session client structure with gboolean interact_2. You can't do this. Unfortunately, that struct is public. If an app is compiled against libxfcegui4 with the smaller structure size, and then libxfcegui4 gets upgraded to a version with a larger structure size, they'll crash. Of course, *most* users of the library probably use _new_full() which will somewhat get around this problem, but, regardless, adding this struct member changes ABI. No good. > The restart, clone etc. commands are set before invoking the application's > save_yourself/save_phase_2 callbacks. SM spec says that interact and > save_phase_2 should SetProperties, and the session client code does than, > but without giving the application a change to set/alter them. And without > that, for example, mousepad will have to set_restart_command() on each > possible filename change (4 calls in callbacks.c and one more in main.c). > So I moved the set_clone_restart_commands() invocation immediately before > SmcSaveYourselfDone(). Yeah, that sounds right. > The --display (if missing) was appended only to the automatically generated > client_session_new() restart/clone commands, and not to any commands set > by the application. So I moved appending to set_clone_restart_commands(). I don't think this is correct. If the caller sets a clone/restart command, the library should send *exactly* what was passed to it. If you want a --display parameter in your clone/restart command, and you're setting one manually, you should add --display. The session client can't assume that the app in question even supports the --display option. (Of course, one could also make this argument about the --sm-client-id argument; but that's just a poor design decision that we're now stuck with.) > client_session_new() initializes both clone and restart commands, so if the > application sets a new restart command, it can not rely on the automatic > restart -> clone copy. Altered client_session_new() to pass NULL, so the > latest restart command is copied to clone. That sounds reasonable, as long as the code will always use the restart command (minus --sm-client-id) as the clone command if clone is NULL. > Added the missing client_session_set_interact_style() function. Currently, > all SessionClient fields are accessed via functions, except for > interact_style which must be directly assigned to. Not a big deal; most session managers seem to ignore the interact style. > Optimizations: > > The largely identical code for setting the different commands in > set_clone_restart_commands() is rebased a single function: > > set_property_from_command(client, command [x11 command name], ptr [command > value], add_sm_id, add_display) > > The identical set_whatever_command()-s are now one-liners invoking the new > copy_command() function. Sounds good. > Notes: > > Using g_strdupv() instead of safe_strvdup() seems to work just fine. Not > knowing why safe_strvdup() stands for, I left it as is. > > IMHO, SessionClient should only include a single session id variable, namely > client_id, which should be equal to the previous id before init and to the > new [given] id after; given_client_id should be a local variable of > session_init(). But I didn't want to break compatibility. Yeah, that seemed to be a questionable design decision from the start; not sure why you'd ever care about the original session id if the session manager gives you something different... and if you do, you can always keep a copy on your own and compare later. But yeah, this needs to stay. > I tried to keep the code simple, since the session management is quite > unpleasant by itself (to avoid a harsher expression). Oh yeah, definitely... > Last but not least, I compiled libxfcegui4.so and replaced my original > shared library. No crashes, and the new library removed --sm-client-id from > the clone commands of xfdesktop and xfwm. Sounds good. Honestly, our session client is pretty awful. The inner implementation is ok (and your patch improves that greatly), but we should really have a new proper GObject-ified XfceSessionClient with a cleaner API, signals instead of callbacks, and of course an opaque struct. But who knows when anyone will have time to do that... so incremental improvements to the current session client will have to do ^_^.
FYI: I'm planning to write a gobject session client for libxfce4ui once i get the the point where the new panel needs session support.
(In reply to comment #2) > FYI: I'm planning to write a gobject session client for libxfce4ui once i get > the the point where the new panel needs session support. A rewrite isn't necessary. Just a simple refactoring and perhaps some improvement of the current session client. You really don't want to write a new session client. Nobody does. Regardless, hopefully we'll be moving away from XSMP as a primary form of session management in the future...
(In reply to comment #1) > (In reply to comment #0) > > There was no way to cancel the shutdown from interact. For compatibility, > > I extended the session client structure with gboolean interact_2. > > You can't do this. Unfortunately, that struct is public. If an app is > compiled against libxfcegui4 with the smaller structure size, and then > libxfcegui4 gets upgraded to a version with a larger structure size, they'll > crash. Not necessarily; probably only if the session client attempts to access interact_2. That is, if an application sets interact style = errors|any (it's none by default), but without providing an interact callback, which seems unlikely. But I have to move the check/call of interact_2 in interact(SmcConn, SmPointer) after the check/call of interact. > Of course, *most* users of the library probably use _new_full() which will > somewhat get around this problem, but, regardless, adding this struct member > changes ABI. No good. Well, xfce panel, xfce4-settings-helper, xfdesktop, xfwm and Thunar seem to work fine. I haven't really seen any xfce session client applications with interactive session save and can't tell about them. > > The --display (if missing) was appended only to the automatically generated > > client_session_new() restart/clone commands, and not to any commands set > > by the application. So I moved appending to set_clone_restart_commands(). > > I don't think this is correct. If the caller sets a clone/restart command, > the library should send *exactly* what was passed to it. If you want a > --display parameter in your clone/restart command, and you're setting one > manually, you should add --display. A caller may set a restart command for something so simple as to put the currently open file name after the program name, and receive it as argv[1] on restart, so a --display may be helpful. OTOH, as you state, > The session client can't assume that the app in question even supports the > --display option. And so then, should I revert the patch to add --display in client_session_new() only, as before, or modify it to never add display?.. > > client_session_new() initializes both clone and restart commands, so if the > > application sets a new restart command, it can not rely on the automatic > > restart -> clone copy. Altered client_session_new() to pass NULL, so the > > latest restart command is copied to clone. > > That sounds reasonable, as long as the code will always use the restart > command (minus --sm-client-id) as the clone command if clone is NULL. Not exactly. With this patch, if an application sets a restart command, and that command contains a --client-id argument, that application should also set a clone command without a client argument. Recognizing and removing argument(s) from a command explicitly set by the user didn't seem a good idea to me.
(In reply to comment #4) > (In reply to comment #1) > > (In reply to comment #0) > > > > There was no way to cancel the shutdown from interact. For compatibility, > > > I extended the session client structure with gboolean interact_2. > > > > You can't do this. Unfortunately, that struct is public. If an app is > > compiled against libxfcegui4 with the smaller structure size, and then > > libxfcegui4 gets upgraded to a version with a larger structure size, they'll > > crash. > > Not necessarily; probably only if the session client attempts to access > interact_2. That is, if an application sets interact style = errors|any (it's > none by default), but without providing an interact callback, which seems > unlikely. But I have to move the check/call of interact_2 in interact(SmcConn, > SmPointer) after the check/call of interact. Yeah, probably. I'd hesitate to keep this, but I guess we can just bump libxfcegui4's soname version. At any rate, libxfcegui4 is hopefully going away in 4.8, so this may not matter. If we refactor this into a decent implementation for libxfce4ui, we can do whatever we want with the API/ABI. A change like this cannot go into the stable 4.6 branch anyway. > A caller may set a restart command for something so simple as to put the > currently open file name after the program name, and receive it as argv[1] on > restart, so a --display may be helpful. OTOH, as you state, Helpful or not, if the app isn't expecting it... > > The session client can't assume that the app in question even supports the > > --display option. > > And so then, should I revert the patch to add --display in client_session_new() > only, as before, or modify it to never add display?.. It should maintain the current behavior for compatibility. Always. > > > client_session_new() initializes both clone and restart commands, so if the > > > application sets a new restart command, it can not rely on the automatic > > > restart -> clone copy. Altered client_session_new() to pass NULL, so the > > > latest restart command is copied to clone. > > > > That sounds reasonable, as long as the code will always use the restart > > command (minus --sm-client-id) as the clone command if clone is NULL. > > Not exactly. With this patch, if an application sets a restart command, and > that command contains a --client-id argument, that application should also set > a clone command without a client argument. Recognizing and removing argument(s) > from a command explicitly set by the user didn't seem a good idea to me. Well, you can't really have it both ways. Currently the session client will attempt to 'do the right thing', and we can't change behavior for compat reasons. Both CloneCommand and RestartCommand are required by the XSMP spec. If the app sets one or the other, but not both, we have to do the best we can to make it work properly. CloneCommand should *never* have a --sm-client-id param in it. RestartCommand *always* needs --sm-client-id. In general, the app shouldn't add --sm-client-id to either, but basically when the session client connects to the session manager, we need to have the behavior I just described. I don't care *how* we get it, it just has to be that way.
(In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #1) > > > (In reply to comment #0) OK then: 1. interact_2 remains, but will only be used if interact style != none && interact = NULL. 2. client_session_new() appends --display-id, for compatibility; set_clone_restart_commands() does not. 3. set_clone_restart_command() appends --sm-client-id to the restart command if missing. 4. set_clone_restart_command() removes --sm-client-id, from the clone command, no matter if derived by the restart command or set by the user. I'll upload it when ready. > Currently the session client will attempt to 'do the right thing' Not 100% - the current (unpatched) set_clone_restart_command() won't remove --sm-client-id from the clone command, and will copy the clone command (if missing) from the restart command 1:1, including --sm-client-id. Thus, xfwm4 and xfdesktop clone commands currently contain client id. In version 4.4.2, xfce4-panel had it as well.
(In reply to comment #6) > OK then: > > 1. interact_2 remains, but will only be used if interact style != none && > interact = NULL. > > 2. client_session_new() appends --display-id, for compatibility; > set_clone_restart_commands() does not. > > 3. set_clone_restart_command() appends --sm-client-id to the restart command > if missing. > > 4. set_clone_restart_command() removes --sm-client-id, from the clone > command, no matter if derived by the restart command or set by the user. > > I'll upload it when ready. Sounds good to me. > > Currently the session client will attempt to 'do the right thing' > > Not 100% - the current (unpatched) set_clone_restart_command() won't remove > --sm-client-id from the clone command, and will copy the clone command (if > missing) from the restart command 1:1, including --sm-client-id. Thus, xfwm4 > and xfdesktop clone commands currently contain client id. In version 4.4.2, > xfce4-panel had it as well. Right... that's why it has bugs. In practical matters, it's not so important, because I'm not aware of a session manager that actually uses CloneCommand, but... better to be correct. FYI, if you're interested to read about some differences in session manager and client implementations, check out: http://live.gnome.org/SessionManagement/NotesOnXSMP Some of it is out of date (xfce4-session 4.6.x fixes a few of the omissions and bugs there), but it's pretty useful, I think.
Created attachment 2369 The updated patch with all comments reflected I used a patched libxfce4gui shared library for the last 24 hours and encountered no problems so far. Thanks for the Session Management Notes link. That was an amusing reading, especially coming from gnome.org, considering my experience with GSM.
Sorry for the delay; I'm finally getting back to this. So here's the thing... libxfcegui4 is deprecated and won't get any releases in the next feature release series (4.8.x). I'll probably do another release of it for 4.6.x, because there's an annoying icon sizing bug in XfceAppMenuItem that should get a stable release fix. However, I'm not too keen on the idea of having to bump the library interface version -- that's a huge pain for distro packagers, since that means they have to relink all packages that depend on it, and in some cases they'll even roll an entirely new versioned package. I don't think this is worth it since no one (except you?) will use the added interact_2 functionality given that the library is a dead-end. So... I'd be happy to check in a patch that just fixes the bugs and problems you found, but I won't include the interact_2 changes. Can you cook up such a patch? For 4.8.x, I'm refactoring the current SessionClient code into a GObject with signals instead of callbacks and a much better API. I've already started on this, and I used the current SessionClient+your patch as a base for this. So your full work will make it into XfceSMClient in libxfce4ui.
(In reply to comment #9) > So... I'd be happy to check in a patch that just fixes the bugs and problems > you found, but I won't include the interact_2 changes. Can you cook up such a > patch? Sure. > For 4.8.x, I'm refactoring the current SessionClient code into a GObject with > signals instead of callbacks and a much better API. I've already started on > this, and I used the current SessionClient+your patch as a base for this. So > your full work will make it into XfceSMClient in libxfce4ui. Glad to hear that.
Created attachment 2515 The patch without interact_2 Also removed client_session_set_interact_style(), since it's (was) a new API and not of much use without interact_2(), and fixed the indentation.
Sorry, thought I replied to this already. The patch doesn't apply to the xfce-4.6 branch. Any idea why?
(In reply to comment #12) > Sorry, thought I replied to this already. The patch doesn't apply to the > xfce-4.6 branch. Any idea why? Yes (and excuse my late answer too). In the svn version I used, supposing it'll be the most recent, session-client.c contains an extra #ifdef HAVE_LIBSM / #endif pair. I'll leave the previous patch too, just in case...
Created attachment 2605 hopefully the last one...
Ok, thanks. Checked in on xfce-4.6 branch, should make it into 4.6.2.
Er, actually close it this time.