[systemd-devel] [PATCH] pam_systemd: new option for the session class

[Lennart Poettering]

On Tue, 27.11.12 23:16, Matthew Monaco (dgbaley27 at 0x01b.net) wrote:

> I don't see any reason why every DM (LightDM for me) needs code to support this.
> 
> It looks to me like its safe to just point to the data in argv, let me
> know if it isn't.

Sounds like a good idea. A few comments though:

> +                                <term><option>class=</option></term>
> +
> +                                <listitem><para>Takes a string
> +                                argument which sets the session class.
> +                                This takes precedent over the XDG_SESSION_CLASS
> +                                environmental variable.</para></listitem>
> +                        </varlistentry>

Hmm, takes precdence over XDG_SESSION_CLASS? I'd always do things the
other way round, i.e. that the runtime param overrides the statically
configured param.

> +                } else if (startswith(argv[i], "class=")) {
> +
> +                        if (class) {
> +				*class = argv[i] + 6;
> +                        }

No {} for single-line if blocks please... This is not PHP ;-)

> -        class = pam_getenv(handle, "XDG_SESSION_CLASS");
> +        if (isempty(class))
> +                class = pam_getenv(handle, "XDG_SESSION_CLASS");

Please invert this logic, see above...

Lennart

-- 
Lennart Poettering - Red Hat, Inc.