Yes, this sounds like a great idea.  Would you be willing to put together a patch that prototypes how you think this should work?

So I'd like to do this incrementally if possible.  The first step should be to try to isolate functionality.  An obvious thing is to try to eliminate the reliance on global variables.  Also, perhaps we should make each slave start as a fork/exec (via gspawn) instead of just a fork.  This will require us to have a clear separation between the master and the slaves.

Once that is done it should be a lot easier to use gobjects.

This sounds good.  I'm happy with the cleanup work you are doing.

However, I think bug #336549 is probably the most important bit of cleanup work pending, so if you could tackle that with your gobject reorg, that would be great.

Some of the issues in the above bug have been addressed, but if you read the last few comments (mainly comment #23), you will see the issues that remain.

Actually, I am working toward fixing that :)  The best way to fix it is by reworking the code to use a Glib mainloop.  And then to use something like the signal handling that is done in the master.

Created attachment 85398 [details] [review]
patch

This patch:

Adds a common logging system ./common/gdm-log.[ch] to be used by both the daemon and child processes.

It works by settings a glib log handler that sends output to both syslog and stderr.  This has a number of advantages over what we have now:
 * use exactly the same mechanism for gui, master, slave
 * the glib stuff is way more robust
 * will be able to log warnings and errors generated in the toolkit or other libraries
 * more threadsafe etc

Other changes in the patch include:

 * separating gdm.h into its component parts.  For example, the GdmDisplay definition really belongs in display.h etc.

 * adds a few more convenience functions to gdm-config.

 * uses getters/setters for the display list instead of using a global.

OK to commit?

Created attachment 85419 [details] [review]
updated patch

The log stuff is the same as the last patch but this removes even more global variables, removes some unused code, cleanups etc.

OK to commit?

Created attachment 85588 [details] [review]
patch

On top of the other changes.  This patch cleans up the ipv4/6 handling in the daemon.  It was a mess before with two separate code paths.  This isn't necessary on modern systems.  I've fixed the code to be mostly AF- independent.

I haven't fixed the gdmchooser gui side yet.  It is still a horrible mess.

Brian, may I please commit this?

Note I added back the gdm_clearenv_no_lang function to daemon/misc.c since
Takao made use of the function in his enhancement bug #108820.  Since you
removed this function in earlier cleanup, I wanted to highlight this to you.

Sorry it took me so long to review your patch.  I was on vacation the 
last half of the past week - also the patch is huge.  In my source tree
I applied your patch and ran "svn diff -x "-w -u" > newdiff to generate
a diff report ignoring white space differences.  This reduced the patch
size by more than half and made it easier for me to review.  In the future, when
you submit large patches that have so much white space changes mixed in with
functional changes, I'd appreciate it if you could also attach a diff file ignoring white space change so I can have an easier time reviewing the changes.

I did not apply your patch to SVN head.  The patch looks good, you can apply it after you address the following issues:

+ Please do not change the default value of the Browser key to true.  If you
  think this sort of interface change is desirable, please open a separate
  bug report and discuss rather than burying this sort of change in an
  unrelated code cleanup patch.  I think changing the default value could
  have negative ramifications on some users.

+ Note that the docs for the GDM_SUP commands are in two places.  In
  the header files and also in docs/C/gdm.xml.  It is a pain maintaining
  them in two places.  Since we are cleaning up anyway, perhaps we should
  remove the docs from the header files and just refer the reader to
  review the docs/C/gdm.xml file, the generated yelp docs, or the docs
  at http://www.gnome.org/projects/gdm/docs.html to see how to use the
  GDM_SUP commands.  What do you think?

+ Could you explain what the value add for the new gdm_config convenience
  functions is?  It isn't clear to me what these are for from looking
  at the code.

+ I notice you changed the function names of the gdm_error_box* and
  gdm_errorgui_* functions.  Could you explain why the function names
  changed?

Also, William, I notice that you modified the code to expand LOCALSTATEDIR
before using it to find the PID file location.

This causes a problem on Solaris because we set localestatedir to /var/lib.  This is needed to cause the GDM files to be installed to the locations recommended in the docs (/var/lib/gdm for authdir and /var/lib/log/gdm for logdir).  However, we really want /var/run/gdm.pid to be installed to /var/run/gdm.pid, and not to /var/lib/run/gdm.pid.

Any ideas of how we should best fix this?  I think just hardcoding to /var/run/gdm.pid should be okay since this directory is standard in UNIX.  Or should we fix the configure so that you add a separate configuration option to specify the location of the stuff that goes into /var/lib?

Thanks for the review Brian.  Hope you had a nice vacation!

(In reply to comment #8)
> I did not apply your patch to SVN head.  The patch looks good, you can apply it
> after you address the following issues:
> 
> + Please do not change the default value of the Browser key to true.  If you
>   think this sort of interface change is desirable, please open a separate
>   bug report and discuss rather than burying this sort of change in an
>   unrelated code cleanup patch.  I think changing the default value could
>   have negative ramifications on some users.

Oops, this snuck in from another patch.  I'll create a new bug for it.

> + Note that the docs for the GDM_SUP commands are in two places.  In
>   the header files and also in docs/C/gdm.xml.  It is a pain maintaining
>   them in two places.  Since we are cleaning up anyway, perhaps we should
>   remove the docs from the header files and just refer the reader to
>   review the docs/C/gdm.xml file, the generated yelp docs, or the docs
>   at http://www.gnome.org/projects/gdm/docs.html to see how to use the
>   GDM_SUP commands.  What do you think?

Yeah I agree.  I've removed them from the header and added a link to the docs.  One thing that might help is having a "latest" directory so we don't have to hard code the version, like:
http://www.gnome.org/projects/gdm/docs/latest/controlling.html

> + Could you explain what the value add for the new gdm_config convenience
>   functions is?  It isn't clear to me what these are for from looking
>   at the code.

I think you are referring to the new gdm_config_*_for_id() functions.  Those are really nice because you can use them to get/set values by specifying only the numeric ID of the option instead of a key/value pair.  Using numeric IDs is a lot nicer than passing around the old *_KEY_* constant strings because you can do switch(){} statements to handle them instead of doing a ton of strcmp()s and strsplit()s.

> + I notice you changed the function names of the gdm_error_box* and
>   gdm_errorgui_* functions.  Could you explain why the function names
>   changed?

I just added some "namespace".  I think it is much easier to read the code when you know where a function, object, or type is defined.  As an aside these functions are really broken and cause a number of problems.  I'll detail that in a later cleanup.


(In reply to comment #9)
> ideas of how we should best fix this?  I think just hardcoding to
> /var/run/gdm.pid should be okay since this directory is standard in UNIX.  Or
> should we fix the configure so that you add a separate configuration option to
> specify the location of the stuff that goes into /var/lib?

Not really sure, but I've changed it to use "/var/run/gdm.pid" as the default as you suggest.  I think that is fine.


I think I've addressed you comments so I'm going to apply this.  We can work on any remaining issues in trunk.

William - couple of issues:

1) daemon/gdm-socket-protocol.h doesn't seem to be in your patches or in
   the source tree.  Could you add this please?  The code no longer builds
   without this.

2) I tried hacking together my own gdm-socket-protocol.h file based on the
   old gdm.h file (copying the GDM_SUP, GDM_SOP, etc. stuff into the file) 
   hoping this would allow me to workaround.  However, the code doesn't compile
   complaining like this:

"gdm.c", line 1897: undefined symbol: GDM_SLAVE_NOTIFY_RESPONSE
"gdm.c", line 2567: undefined symbol: GDM_SOP_CUSTOM_CMD
"slave.c", line 1182: undefined symbol: GDM_SOP_SHOW_ASKBUTTONS_DIALOG

   You don't mention changing these symbols, or why this was needed.  Could
   you please explain these changes?  Or is this just because I didn't make
   my header file correctly?

3) Solaris doesn't support __PRETTY_FUNCTION__, can you use 
   G_GNUC_PRETTY_FUNCTION instead?

4) Solaris doesn't support LOG_PERROR in openlog().  Should fix like this, or
   just use LOG_ERR instead of LOG_PERROR always?

#ifdef __sun
        openlog (prg_name, LOG_ERR|LOG_PID, LOG_DAEMON);
#else
        openlog (prg_name, LOG_PERROR|LOG_PID, LOG_DAEMON);
#endif

5) Unrelated to the above, but I'd really like to fix the PidFile deprecation
   in the 2.18 branch as well.  Would you be willing to backport that fix?

OK, those should be fixed now.  I'll try to look into 5.

William, note I had to make a minor modification to daemon/xdmcp.c to get the code to compile on Solaris.  This is because on Solaris, libwrap requires the calling program to define the two following variables - otherwise the linker complains the symbols are undefined.  If you think it's better to move this code to daemon/gdm-log.c and surround it by #ifdef HAVE_LIBXDMCP, then let me know and I can change this, or feel free to do so yourself if you want.

---

/*
 * On Sun, we need to define allow_severity and deny_severity to link
 * against libwrap.
 */
#ifdef __sun
#include <syslog.h>

gint allow_severity = LOG_INFO;
gint deny_severity = LOG_WARNING;
#endif

--- 

Otherwise, after your last commit, everything is working and building fine on Solaris.  I also had to fix a bug in daemon/gdm.c where you broke the GET_CONFIG_FILE gdmflexiserver option.

Also, thanks for looking into #5.

William.  I went ahead and backported your PID_FILE fix to 2.14, 2.16, and 2.18.  I think this is a big improvement for users because it is nice to have the dialog display propery and quickly.  Also, since this read of the debug key wasn't in the gdmcomm_bulk_read_start/stop block, this was causing an additional socket connection to the daemon to get the key value.  So GDM should be a tad bit faster without accessing this key here.

I do have one question.  When you modified the code to deprecate GDM_KEY_PID_FILE and GDM_KEY_ALWAYS_RESTART_SERVER, you didn't remove all the places where GDM_KEY_PID_FILE is referenced in the code.  I see it only used in one place, gdm-daemon-config.c, and it just returns a flag saying "I won't set this key".  This seems pretty useless.  Shouldn't we remove this code and the definition in the header file (gdm-daemon-config-keys.h)?

It is actually used in two places: gdm_daemon_config_to_string, gdm_daemon_config_update_key.

I left these because they are used to handle the socket interface.  Removing it from there might have negative effects on users of the socket (ie. fast-user-switch-applet).  So, I don't think that would be appropriate for the stable branches but might be ok for trunk.

I'm confused.  You don't actually reference the GDM_KEY_PID_FILE in gdm_daemon_config_to_string.  Instead you have this code:

        /* Backward Compatibility */
        if ((strcmp (group, "daemon") == 0) &&
            (strcmp (key, "PidFile") == 0)) {
                result = g_strdup (GDM_PID_FILE);
                goto out;

It might be better if you *did* use the GDM_KEY_PID_FILE instead of
hardcoding the strings as you do above.  Either way, though, the above code should make "GET_CONFIG" work properly, which is what is needed for the socket interface and programs like fast-user-switch-applet, etc.

The only place you use the key name is in gdm_daemon_config_update_key.  All this code does is return "false" meaning you can't change the key.  What's the difference of just removing the key completely so when you call UPDATE it just returns "key not valid"?  It's just a different error message.

The *only* place it makes sense to reference this key is in gdm_daemon_config_to_string, if it is used anywhere.

---

By the way, I notice a number of bugs when using GDM now, I think related to the work you have done recently:

1) I see these errors when I run gdmsetup with debug turned on:

** (gdmsetup:24720): DEBUG:   Got response: 'ERROR 50 Unsupported key <greeter/Welcome[C]>'
** (gdmsetup:24720): DEBUG:   Got response: 'ERROR 50 Unsupported key <greeter/Welcome[C]>'
** (gdmsetup:24720): DEBUG:   Got response: 'ERROR 50 Unsupported key <greeter/RemoteWelcome[C]>'
** (gdmsetup:24720): DEBUG:   Got response: 'ERROR 50 Unsupported key <greeter/RemoteWelcome[C]>'
** (gdmsetup:24720): DEBUG:   Got response: 'ERROR 50 Unsupported key <greeter/GlobalFaceDir=/usr/share/pixmaps/faces/>'
** (gdmsetup:24720): DEBUG:   Got response: 'ERROR 50 Unsupported key <greeter/GlobalFaceDir=/usr/share/pixmaps/faces/>'

   Note sure what the problem with greeter/GlobalFaceDir is.  Perhaps
   the problem with the Welcome and RemoteWelcome is that you didn't
   code the stuff to make the config file work with translatable strings?

2) It seems that when I try to Shutdown, Restart, or Reboot from the
   login screen, it doesn't work - just returns me back to login screen.

3) Likewise running gdmchooser from the login screen (Remote login via
   XDMCP) doesn't work - just returns me back to the login screen.

Do you see these problems?

Also, William, in our discussion I thought you were going to go ahead and apply the patch for bug 326771 as a part of deprecating AlwaysRestartServer.  Did you do this, or should I go ahead and integrate this patch into 2.19?

I've committed a partial fix for item 1. in Comment #16.  I'm looking into the others.

Ok, items 2 and 3 from Comment #16 should be fixed now too.

I haven't had a chance to look at bug 326771.  So, you should probably do it.

Note, I made a minor modification to your changes.  In daemon/gdm-daemon-config.c and common/gdm-config.c you were printing out locale stuff in debug calls.  Sometimes the locale value is NULL, and Solaris SEGV's if you try to call printf functions with NULL data.  This is now fixed.

Created attachment 86296 [details] [review]
turn xdmcp into a gobject

This moves the functionality that was in xdmcp.c into a GdmXdmcpManager object.  The interface to xdmcp.c is kept the same (ie gdm_xdmcp_init, _run, close) so that the changes to the rest of the code are minimal.  This was pretty simple because it already is built around the glib mainloop.  OK to commit?

Looks good - you can commit.

I found another bug in the new configure code.

I created an /etc/X11/gdm/custom.conf:0 file with the following entry:

[greeter]
SystemMenu=true

Now that I have a display-specific configuration file, it seems that all boolean values don't default properly (integers like greeter/MinimalUID and strings like greeter/Exclude seem to work okay).  In short, this means that the GUI looks really bad once I have created the above per-display configuration file (no background color, no titlebar, etc.)

In other words, if I run:

gdmflexiserver --command "GET_CONFIG greeter/TitleBar"

it returns "true" which is correct (since this key is only defined to be true in the defaults.conf and is not in custom.conf or custom.conf:0)

However, if I run 

gdmflexiserver --command "GET_CONFIG greeter/TitleBar :0"

it incorrectly returns "false" as if it is only looking in the custom.conf:0 file and not defaulting back to checking the value in the custom.conf file or defaults.conf file.

Yup, that's a bug.  I've committed a patch that should fix it.

Some issues with the latest patch:

I notice that gdm-xdmcp-manager.h now has this line:

#include <dbus/dbus-glib.h>

which breaks the build if you aren't building with ConsoleKit enabled.

I also had to add this line to gdm-xdmcp-manager.c:

#include <net/if.h>

to get it to find ifconf, ifc_len, ifreq, ifc_buf, ifc_name, etc. symbols.

Also, the code no longer compiles for gdm-xdmcp-manager.c with these errors on Solaris:

"gdm-xdmcp-manager.c", line 501: undefined symbol: SIOCGIFCONF
"gdm-xdmcp-manager.c", line 515: undefined symbol: SIOCGIFFLAGS

I notice that these have the following comments.  To get "compatibility mode", you need to compile with -DBSD_COMP.  Would it be better to use lifreq here?

/*
 * Obsolete interface ioctls using struct ifreq that are supported
 * for compatibility. New interface ioctls use struct lifreq.
 */

In my private build I went ahead and added -DBSD_COMP when building gdm-xdmcp-manager.c to work around this and don't see any other problems building the new code.

Ok, I think I've fixed these in SVN.  Thanks for testing.

The SIOCGIFCONF etc code was copied basically unchanged from xdmcp.c so I think it should be fixed by adding back some missing includes - but I'm not able to test on Solaris.

Thanks, your latest changes now compile okay.

William:

A few issues with the new config logic:

1) You have the function gdm_config_value_get_string_array and you are using 
   this for just the HALT, REBOOT, and SUSPEND keys.  The code seems to be 
   hardcoded to only support the ";" as the delimeter.

   However there are many keys that are "arrays" and that also use different
   delimeters.  These include:

   Halt/Shutdown/Syspend (uses semicolons)
   SystemDesktopDir (uses colons)
   ConsoleCannotHandle (uses commas)
   Include (uses commas)
   Exclude (uses commas)
   GraphicalThemes (uses "/:")

   I'd prefer that GDM either support all the above as "arrays" or not support
   a special array type at all.

   Perhaps the gdm_daemon_config_entries array needs to be enhanced so the
   delimeter can be specified or have separate types like 
   GDM_CONFIG_VALUE_STRING_ARRAY_COLON, etc.

   Note Path, RootPath and GtkModulesList are also "arrays" but are not treated
   as arrays by GDM.

2) I notice running gdmflexiserver --command "QUERY_LOGOUT_ACTION" fails 
   with these messages:

   WARNING: Request for configuration key daemon/HaltCommand=/sbin/init 5, but 
   not type STRING

   So I don't think you coded the QUERY_LOGOUT_ACTION function to work with 
   these as arrays.  Please be more careful when you make things ARRAYS to
   fix all the places where the key is referenced.

3) Maybe we should get rid of the "legacy" keys and just reference the new
   ID style keys as defined in gdm-daemon-config-entries.h?

For #2 above:

I should say that it seems that it seems that QUERY_LOGOUT_ACTION and SET_LOGOUT_ACTION are broken because gdm.c thinks they the HALT_COMMAND, etc. are null.  I think the problem is that they are listed as string_arrays but the functions processing QUERY_LOGOUT_ACTION/SET_LOGOUT_ACTION/SET_SAFE_LOGOUT_ACTION are treating them like strings?  So it seems that halt, shutdown, suspend is still broken.

1)
For the string array types the delimiter isn't so much hardcoded as it is specified by the spec:
http://standards.freedesktop.org/desktop-entry-spec/latest/ar01s03.html

We were already using ";" as the delimiter for the HALT, REBOOT, and SUSPEND keys so it seemed natural and a bit more robust to let GKeyFile handle the string splitting for those keys.

For the others:
   * SystemDesktopDir (uses colons)

     I think of this more like a PATH type variable and not really an array of strings type.  The docs seem to confirm this: http://www.gnome.org/projects/gdm/docs/2.18/configuration.html

   * ConsoleCannotHandle (uses commas)
   * Include (uses commas)
   * Exclude (uses commas)

     These are arrays of strings.  I propose that we change them to use semicolons by default and deprecate the use of commas.  We can handle the commas in a backward compatible way if we detect that no semicolons are present in the value.
 
   * GraphicalThemes (uses "/:")

     This too is an array of strings.  I propose that we handle it just like the above.


2) 
I'll fix it.  Yeah, sorry about that.  Hopefully after all this refactoring the interactions will be a lot less complex and more well contained.

3)
Yeah, probably.  One of the obvious problems with the way those "keys" are used it that they aren't just keys but also group names and default values.  The fact that the default value may or may not be used makes the interactions far more complex then they should be.

Note that GKeyFile lets you change the delimiter from ; to , or whatever  (I had to do this to support the icon theme spec which weirdly used , instead)

I am agreeable with deprecating the delimeter to use ";" or whatever more consistantly as long as the solution is backwards compatible with the keys that currently use other delimeters.  Also, if we are going to support string arrays in the daemon, we should probably also support it in the GUI clients, so that when gdmgreeter (for example) when it wants the Exclude key it gets it in a similar fashion.  This probably requires enhancing gui/gdmconfig.c a bit.  I know you opened the other bug about whether the GUI's should interact with the config file directly - if we go in this direction then perhaps that will also solve this problem?

By the way, I fixed the problem with QUERY_LOGOUT_ACTION, SET_LOGOUT_ACTION, and SET_SAFE_LOGOUT_ACTION.  I did this because I needed to add RBAC support for these options, which you can also review in SVN head.

I notice another bit of oddness.  When I run gdmsetup and modify a parameter, it looks like a bunch of carriage returns get added to the file.  Might be nicer if the file didn't grow like this.  I know it didn't used to.

William - note that GDM 2.19.1 release is next Monday, so please make sure to give things a extra test and lets try to make sure the code is in as reasonable shape as possible before then.  If you find any bugs, feel free to commit fixes.  Though lets try to avoid adding any more significant code changes before the 2.19.1 release.  Okay?

Yes, completely agree.

Any updates on the status of this bug?  William, I think I'd appreciate it if you were able to perhaps give a roadmap on what future changes you plan/hope to make in this area.  I'm guessing the long-term goal is to integrate D-Bus more into GDM, but I'm unsure how much work is involved or how close we are.

First, the things to do:

The next step is to separate the logic of the slaves from the daemon by using fork/exec instead of just fork.  This will allow D-Bus to be used properly from the slaves since it doesn't really work well in the fork only case (relies on FD_CLOEXEC etc).  Besides making it easier to support D-Bus, I think it is much easier to audit and control what gets copied to the child (eg. environment, data structures, and file descriptors).

This means that we have to rethink how information is passed to the slave from the master.  Currently the fork makes a copy of the Display struct for the slave.  Obviously, this won't work.  Which in a way is a good thing since the Display currently is complicated by a number things that are used by only one of the master or slave.

We also have to rethink how we use the SOP interface.  We use the duplication of FDs after the fork to give a the slave a socket to use.  That won't work either.

We also need to move the SUP handling into an object.  However, doing so reveals that there is a lot of globally scoped stuff that needs to be fixed.

The management of the list of displays needs to changed.  We have a better way than each component just modifying a GSList.


Unfortunately, we are at the point where it isn't easy to make any of these changes individually.  I've made fairly good progress on the changes.  I'll add another comment discussing this plan in a few hours.

Since talking about this abstractly will be very difficult.  I've committed the skeleton of these changes to a branch:
http://svn.gnome.org/viewcvs/gdm2/branches/mccann-gobject/

There is still quite a bit of work to do but this should give you a pretty good idea of where I'm headed.  It doesn't quite work yet but should (barely) compile.

Brian & William:

Just a heads up, I think I found a couple of bugs in the "patch" patch; please take a look here:

https://bugzilla.novell.com/show_bug.cgi?id=296699#c9

Peace!

Nice catch.  I've committed the loopback bit to the gobject branch.  Should also go into trunk.

Thanks, fixed in SVN head.