Created attachment 49724 [details] [review]
patch

Look ok?

I think this patch needs work.

1) It isn't cool for a GDM program to launch xserver using PATH.  For security
   reasons (to avoid trojans), the full PATH the the screensaver should be
   specified, perhaps in the config file instead of depending on the PATH. 

2) The code shouldn't have hardcoded logic to choose one
   screensaver over the other.  Instead, it should be specified in the 
   gdm.conf file, and should be specified with full-pathname to avoid 
   trojan attacks. 

3) The configure script should check to see which screensavers are installed 
   and can make gnome-screensaver the default in gdm.conf if it is present.  
   It would also be nice to have a configure option to specify the full path 
   of the screensaver to use (and thus avoid the checking).  This is useful
   if a person has the screensaver installed to a non-default location
   that the configure script wouldn't know about.

 

1. OK, I can make it use the full path.  Note that the way it is now relies on
PATH too.

2. What other screensaver programs are you hoping to support?  This is exactly
the same logic that gnome-panel and gnome-session use to detect the screensaver. 

3. We can't check for screensavers in configure because that would add a build
time dependency on them.  This would result in a circular dependency.  The
screensaver must be detected at run time.  I don't think adding a dependency in
gdm.conf is the right way to go either for similar reasons.

The screensaver should be able to be switched out without rebuilding gdm or
changing the gdm configuration.  In other words, it should be detected at run
time.  I don't see any problem "hard coding" a list of known screensavers that
can be detected.  Honestly, there are really only two.


The right way in the longer term will be to develop a common Dbus interface to
the screensaver.  So, any screensaver that is registered for the
"org.gnome.screensaver" service can implement a setActive(bool active) method.

In fact, gnome-screensaver-command is already just a wrapper around this Dbus
interface.

What do you think?

Yes, I realize that the current code depends on PATH.  That isn't a good idea,
though, for security reasons, and should be fixed.

GDM shouldn't use the same logic as gnome-panel and gnome-session since GDM
is also used by people who don't use GNOME, such as KDE.

I'm agreeable to checking for screensavers at runtime.  What I recommend doing
is setting up a gdm.conf configuration variable where you can list the
screensavers, separated by a "," character.  Then at runtime, we loop through
them and try them one at a time.  This way the sysadmin can control which
screensaver's are used.  We can set the default value to try gnome-screensaver
first, then xscreensaver.  Users who are using KDE, or their own screensaver,
might want to customize this list.  This might be easier than a Dbus interface.

The default list of screensaver's should be full-path for security reasons.  
It might be nice to add a configure option so the user can specify this list at
configure time, rather than having to customize the gdm.conf by hand after it is
built, or patching it.

After discussion, it seemed to make the most sense for GDM to work like other
GNOME programs, so the same code for launching gnome-screensaver used in other
GNOME programs is now used in GDM.

I think this version is a little bit better than Rodrigo's patch since it
doesn't use deprecated functions or duplicate code.  I'll update it for HEAD.

Created attachment 52742 [details] [review]
updated patch

Okay, please update it in CVS head.  Actually if you want you can put this 
patch in the 2.12 branch since I plan to do a 2.8.0.5 release sometime soon
and it would probably be nice to have this available sooner than 2.14.
Though if you do provide this patch in the 2.12 branch please change the 
g_warning message strings so that they are the same as they are in the old code
since the l10n people won't want to update the strings for this branch.

OK, thanks.  Committed to head and gnome-2-12.