Yes, probably would be good to use glib/gtk+.  I have done a lot of work over the past year to make sure that all vicious-extensions code is only referenced in daemon/gdmconfig.c and gui/gdmconfig.c.  So the code is a lot cleaner than it was a year or so ago when the vicious-extensions code was references pretty much everywhere in the GDM code.  So that should make it easier to convert to something else.

This might be a tricky change, though, if vicious-extensions provides some features that aren't in glib/gtk+/

Note the GKeyFile api (included in glib) should be able to read and write ve_config files.  The formats are very similiar.

I would accept a patch to remove vicious-extensions from GDM daemon, assuming it works and is well written.

FYI: I'm working on this now.

Created attachment 85017 [details] [review]
patch 1

Phase I.

* creates a ./common directory in which to build libgdmcommon.a
* add a convenience layer over GKeyFile for dealing with compound keys and default values
* replace selected ve functions like ve_split
* drop the glade helper stuff
* copy most of the remaining ve stuff into libgdmcommon.a (may replace later)
* fix some leaks

I've tried to make the patch as minimal as I could.  And I'm dogfooding this now so at least the basics work.

Brian, would it be possible to branch GDM so we can commit this and make further improvements against SVN?  I'm a little worried about this getting out of sync.

Very nice patch - this looks awesome.

I applied your patch to SVN head, now that we are in GNOME 2.19, I am okay with head being a bit unstable.  I'm also very interested in moving away from vicious extensions and I think this patch looks quite good.

One thing I'd like you to fix, though.

- I really don't like that you defined an identical function hig_dialog_new
  in every single program (10 copies of it in total).  Could you move this
  function into the common library, or into gui/gdmcommon, or somewhere
  and only define it once.

Also, keep an eye out for some functions that are duplicated in the daemon
and the GUI's.  I think there may be some duplicated code because there
previously wasn't a common library shared between the daemon and GUI's
(aside from vicious-extensions and it sort of didn't make sense to put
GDM specific stuff in there since it was a separate module).

Could you also explain what you think Phase II, III, etc. will be or could be?
I'll leave this bug open until I hear from you that you are done implementing phases.

*** Bug 420649 has been marked as a duplicate of this bug. ***

Great!  Thanks Brian.

So, the copying of the dialog code was a temporary measure.  I didn't want to add it to libgdmcommon because it would add a gtk+ dep that some other areas of the code might not want.  I would like to turn ./gui/gdmcommon.[ch] into the graphical version of libgdmcommon.

So that is part of the phase 2.  Right now we have a number of places where we have utility code:
./common/*
./daemon/misc.[ch]
./gui/misc.[ch]
./gui/gdmcommon.[ch]

I'm working on a patch to sort this out a bit better.

Excellent.  Also I think there is probably some code duplicated in gui/gdmuser.c and the daemon.

is that hig_dialog even needed anymore? I was under the impression that recent gtk finally fixed gtkmessagedialog to be hig compliant...

There isn't really much to that function anymore.  In the old days it did more work.  Now the function is literally only 5 lines.  That's another reason why I wasn't too upset about copying and pasting it.

Brian, it seems like maybe you didn't add the common directory to trunk.

I went ahead and added the missing files.

Thanks William.  Sorry about that.  

I notice when I check out gdm2 that the vicious-extension propset seems to still make GDM want to pull the vicious-extensions directory.  I tried running propdel on it, but it didn't go away.  Do you know how to get rid of the propset so vicious-extensions isn't added to GDM anymore?

Created attachment 85205 [details] [review]
patch 2

Phase 2

Remove a few more of the copied VE functions.  But the major change is a replacement for daemon/gdmconfig.[ch].

This adds common/gdm-config.[ch] that does a few things:
  * allows for transparently having mandatory/default/custom configuration sources (mandatory isn't used ATM)
  * maintains a database/cache of items
  * has a mechanism for specifying which items should be cached/loaded
  * provides a variant data type (GdmConfigValue)
  * provides a mechanism for specifying information about config items like:
     group, key, default-value, datatype, shortcut id
  * a simple notication callback for when items of interest change
  * a simple validation callback that can be used to modify or validate the values after loading, before they are used or sent to the notication callback.

This gets rid of a ton of special casing and very complicated code.

I'm dogfooding this now.  What do you think?

Should also mention that this includes a ./common/test-config that demonstrates and tests the new API.

William:

Overall the patch looks really good.  I notice the patch doesn't update the
docs at docs/C/gdm.xml.  Is this because the functionality hasn't changed at
all, just the internals?  If your modifications affect end-use at all, I'd
expect to see some doc updates.  Does your new mandatory/default/custom/
built-in/runtime-user breakdown of configure keys affect usage at all that
should be documented?

Also note the docs in docs/c/gdm.xml reference the "gdm.h" file in a few places 
saying (read comments in the header file for more information).  Since you have 
reorganized this file, I'd expect some of these pointers to change, or perhaps 
the docs should be moved/copied into docs/C/gdm.xml so we don't reference a 
header file in the docs for more information?  It's probably okay to reference 
the header file for tasks that require compiling against the gdm.h header file 
(using the sockets protocol directly, for example).

In your new daemon/gdm-daemon-config-entries.h file I see these comments:

+ * + Update the config_options[] to add the
+ *   new key.  Include some documentation about the new key, following the
+ *   style of existing comments.

Please specify where this config_options lives.  It isn't in the same file.

+ * + Add any validation to the _gdm_set_value_string, _gdm_set_value_int,
+ *   and/or _gdm_set_value_bool functions (depending on the type of the new
+ *   key) in gdmconfig.c, if validation is needed.

Didn't you remove these functions?  Do these steps need further updates to
reflect how you have changed things?  I think so.

Also your patch doesn't apply cleanly - I get errors that daemon/gdm-daemon-config.[ch] don't exist.  Shouldn't these two files be diff'ed against /dev/null.  Once you address this issue, feel free to apply the patch.

---

Also, there are two changes in the works that affect configuration.  I was planning on making these changes, but since you are rewriting the internals for
the config logic, I'd feel a bit more comforable if you could integrate these
two changes.  They are both minor bits of work.  Would you be willing to do
this?

1) See bug #326771.  This adds a feature so that additional Xserver arguments
   can be specified in the desktop file.  This way if you want to create a
   session that requires additional Xserver arguments (e.g. additional 
   Xserver extensions), then you can do so.  The patch in the bug report adds
   this feature, but might require a bit of rework since you changed the 
   functions for accessing configuration keys.

   However, in discussion we also agreed that it makes sense to deprecate the
   AlwaysRestartServer key.  So probably would be good to remove all references 
   to it in the code, except when you run the GET_CONFIG command it should 
   always return "true".  Probably should also leave it in config/gdm.conf.in
   file with a new comment that says it is deprecated and always true.

   This key is only used in daemon/slave.c in one place (slightly modified
   by the patch).

2) See bug #162849.  We should also remove PidFile configuration file and 
   just always hardcode the PidFile to /var/run/gdm.pid.  This way the clients
   can find the daemon pid without needing to call configuration first (which
   requires going over the socket for configuration).

   Again, we probably should fix the GET_CONFIG command to always return
   "/var/run/gdm.pid", leave it in config/gdm.conf.in file with a new comment
   that says it is deprecated and always set to this value.

   Also, obviously need to modify the logic in the daemon and the client
   programs to just use a single #define for the file location rather than
   finding out the filename via configuration.

   To test this simply move your /tmp/.gdm_socket file away and move your
   /var/run/gdm.pid file away.  Then when you run a program like gdmflexiserver
   it should respond with a pop-up saying that GDM isn't running instead of
   getting a bunch of "Failed to connect to socket, sleep 1 second and retry"
   errors.  When done testing, I'd move the /tmp/.gdm_socket and 
   /var/run/gdm.pid files back into place (or restart GDM).

If you could fix these, also feel free to commit them when done.  Though I'd
apply them as separate commits from the gdmconfig cleanup.

---

Another issue to think about:

Note that in the config/gdm.conf.in file that we have defaults in the comments.
So you see lines like:

#KillInitClients=true

The idea is that the comment should show you the compiled-in default value.  
However, it is very easy for the code where the compiled-in default lives and 
the comment in gdm.conf.in to get out-of-sync.  It might be nice if we didn't 
have to maintain the values in two places.  It would be better, I think, if
when building GDM if the gdm.conf file got built with the actual defalt values 
defined in the header files.

This would avoid problems where a distro patches a GDM default value in the
wrong way.  Wrong ways include:

+ Just patching the gdm.conf.in file so the line is uncommented and has a
  different value, but not the default in the header file.  This means if the 
  user comments out the line, they get the wrong default.
+ Just patching the header file, but leaving the comment wrong.  This is
  worse since the comment implies that the behavior is different than it
  really is.
+ It would also avoid issues where the two values are just simply out of
  sync.

This might be more trouble than its worth, though.  I recently reviewed the
gdm.conf.in file and I think they are all right (assuming distros don't
patch the values incorrectly).  Just would be nice to have the default 
values defined in one place rather than two.  I just wanted to mention this
other issue since you are cleaning up this code.

Also might be nice to tackle bug #333327.

OK, I've fixed these issues and committed the patch.  I've also committed patches that fix most or all of 1) and 2) above.

I'm going to close this since the VE part is done.  I'll continue work on bug #376010.

These patches didn't migrate verify-shadow.c and verify-crypt.c to the new API, breaking build when using one of those backends for authentication.

Should I open a separate bug report for this ?

This should be fixed now in SVN.  Can you verify?  Out of curiousity, are you really using those backends in production?

Confirmed, both backends build fine now.

Don't worry, I was using crypt only in jhbuild because I was lazy to configure pam support some years ago and since them, I kept using the same jhbuild config as a basis for each branch :)

Thanks for testing. :)