Re: [gdm-list] A review of patches in Debian packaging

[Josselin Mouette <joss debian org>]

Le mardi 06 juillet 2010 à 11:58 -0500, Brian Cameron a écrit : 
> > 01_minimal_uid.patch
> >          Debian-specific. Non-system UIDs start at 1000 for us.
> >          What would be nice would be a way to specify this information as
> >          a configure option.
> 
> The old GDM allowed sysadmins or distros to configure this in the GDM
> daemon configuration.  Perhaps it makes sense to add this configuration
> option back so distros can set it as needed in their default
> configuration file.

There are already #598368 and #623225 about this topic.

> > 02_xnest-wrapper.patch
> >          Instead of hardcoding the Xnest/Xephyr choice at compile time,
> >          we replace it with a script in /usr/share/gdm that checks for
> >          their presence at run time.
> >          Anyway this is useless currently since nested logins are still
> >          not available.
> 
> Seems reasonable, but not sure how much value it has until the new GDM
> supports Xnest/Xephyr style sessions.

I’ve filed #624372.

> > 05_stop_welcome_session.patch
> >          Since the gnome-session and dbus-daemon processes for the login
> >          manager are killed before starting the user session,
> >          --exit-with-session is not appropriate. On the contrary it will
> >          start a watcher process that remains after the user has logged
> >          on.
> 
> +1 to get this patch upstream.  I'd recommend filing a bug in
> bugzilla.gnome.org in the "gdm" category.

#624373

> > 06_first_vt.patch
> >          This patch reintroduces the TTY manager from GDM 2.20 instead of
> >          letting X decide on which VT to run. Letting X decide makes it
> >          start on tty1 and crash as soon as getty does the same. There
> >          was broad consensus among Debian developers that neither GDM nor
> >          graphical boot thingies should not interfere with tty1 to tty6.
> 
> Ray and Jon are more familiar with how the new GDM works with display
> switching, so I'll defer to their thoughts about this.

Waiting for more input.

> > 07_libexec-paths.patch
> >          Various things in the build system suppose that $libexecdir is
> >          the same for GDM and other packages: ConsoleKit, g-s-d,
> >          PolicyKit. This excludes FHS-compliant systems, which don’t have
> >          a /usr/libexec. Currently we hardcode the paths instead, but
> >          this should really be detected with pkg-config.
> 
> Yes, this should be detected with pkg-config.  Using configure options
> should also be acceptable if the information about an install location
> is not available via pkg-config.  I'd think that if this were made
> appropriately configurable, it should go upstream.

Patch submitted to g-s-d: #624377.

> > 08_frequent-users_greeter.patch
> >          To be sure the greeter doesn’t show users that shouldn’t be here
> >          for one reason or another, this patch sets the session type to
> >          "gdm" for user sessions, and requests to CK only sessions
> >          corresponding to this type.
> 
> I'm not clear about what this patch does, or what problem it is trying
> to solve.  Is there a bug report that we could review, or any additional
> information you could provide?

As explained to Ray, it is a safeguard. For example at my work sysadmins
are logging very often to the machines, but you don’t want to see them
on top of the user list.

> > 09_default_session.patch
> >          Use a default session set through alternatives instead of
> >          hardcoding gnome.session. See GNOME #594733
> 
> The bug report highlights that we really should have a configuration
> option for this.  The latest patch for this report doesn't seem to
> address these comments.

My understanding of Didier’s patch is that it does precisely this. What
do you think is missing?

> > 11_xephyr_nested.patch
> >          This is a first attempt by Luca Bruno at re-introducing the
> >          nested login functionality, but xauth is still not right so the
> >          patch is disabled.
> 
> Would be cool to get Xnest/Xephyr support back into GDM, so I hope this
> continues to be worked on.

As it is, the patch works, but it doesn’t start Xephyr with the proper
xauth cookie. Fixing this should be easy for someone familiar with
xauth. I’ve put the patch in #624370.

> > 12_polkit_settings.patch
> >          Based on the Ubuntu patch with a handful of fixes. Allows to set
> >          configuration options from the user’s session, using PolicyKit.
> 
> Again, I think Ray or Jon would be the best to review this.

Waiting for more feedback.

> > 13_gdmsetup_desktop.patch
> > 13_gdmsetup_ui.patch
> > 13_gdmsetup.patch
> >          The configuration tool. Taken from Ubuntu with lots of cleanups.
> 
> Bug #587750 was the bug about adding back a gdmsetup like program.  Jon
> McCann closed this as WONTFIX without really providing much information
> why or how we plan to move forward.  He only said "I think migrating
> our settings to dconf and adding some settings controls to the account
> dialog is the way forward."
> 
> So, not sure what to do here.  If your improvements to gdmsetup are
> significantly better than the patches already in bug #587750, then
> it might be worth attaching the updated patches and re-opening the
> bug to spur some more discussion.

I’ve attached the improved patches. Note that I can’t reopen the bug
since I’m not the submitter.

> > 18_switch_kill_greeter.patch
> > 20_endsession_respawn.patch
> >          As explained in GNOME #618047, user switching leaves a X server
> >          behind, which is not acceptable. It also has inconsistencies in
> >          terms of interface. All of it is built on the premise that X
> >          switches VTs by itself when dying, which is not right when
> >          launched from a display manager. Luckily there is a X server
> >          flag to disable this behavior (-novtswitch) so we have reworked
> >          user experience around it.
> 
> I think Jon or Ray is the best person to look over this.

Waiting for more feedback.

> > 19_configure_xserver.patch
> >          Allows to set X server arguments in the configuration file. See
> >          GNOME #586777.
> 
> A nice feature, though as the bug report says we need to figure out what
> is going on with MultiSeat.  The MultiSeat code already provides this
> sort of configuration, so if MultiSeat goes upstream this patch is not
> needed.

Yes, it’s a small subset of that functionality.

> > 21_schemas_usr.patch
> >          Put gdm.schemas in /usr, it has nothing to do in /etc. The patch
> >          is probably missing the corresponding documentation update.
> 
> If there is a good reason to move the schemas to /usr, then I'd file a
> bug report and explain the reasoning.

Submitted #624379.

> > 22_noconsole.patch
> >          Allows to run GDM without a local X server. This was reported as
> >          GNOME #567522 which was closed but not entirely fixed. Now it
> >          accepts to run if the X server fails to start, but this patch is
> >          more complete and allows to configure it in headless mode.
> 
> I'd file a new bug or reopen 567522 with the latest patch.

The only changes I made to the patch were to make it apply cleanly on
top of the others.

I added a comment to the bug but cannot reopen it.

> > 23_autologin_once.patch
> >          Allow to log in someone after exiting from an autologin session.
> >          See GNOME #587606
> 
> The bug seems to be blocking because it would be better if the behavior
> were configurable.  Some users want autologin to work all the time,
> while others want autologin to work on bootup and then switch to timed
> login.

I agree a configuration option would be better, but the default behavior
is incorrect; I was pointed out that it can have non-trivial security
implications, see http://bugs.debian.org/578736

> > 24_show_user_other.patch
> >          On NIS/LDAP systems with no local user, no user shows in the
> >          chooser, and in this case the “other…” choice disappears. I
> >          can’t think of a reason why this would be useful, apart
> >          preventing people from logging in. The patch makes the “other…”
> >          choice always displayed.
> 
> This looks like a fix that should go upstream.  I'd file a bug.

Filed #624467.

Cheers,
-- 
 .''`.
: :' :      “Fuck you sir, don’t be suprised when you die if
`. `'       you burn in Hell, because I am a solid Christian
  `-        and I am praying for you.”   --  Mike