Re: [libvirt PATCH 1/7] util: Refactor virSystemdHas{Machined,Logind}

[Martin Kletzander <mkletzan@xxxxxxxxxx>]

On Fri, Feb 02, 2024 at 01:57:37PM +0100, Ján Tomko wrote:
> On a Thursday in 2024, Jiri Denemark wrote:
> > To reduce code duplication both function now use a common
> > virSystemdHasService helper.
> >
> > Signed-off-by: Jiri Denemark <jdenemar@xxxxxxxxxx>
> > ---
> > src/util/virsystemd.c | 76 ++++++++++++++++++++-----------------------
> > 1 file changed, 36 insertions(+), 40 deletions(-)
> >
> > diff --git a/src/util/virsystemd.c b/src/util/virsystemd.c
> > index cd4de0eef8..21892aca02 100644
> > --- a/src/util/virsystemd.c
> > +++ b/src/util/virsystemd.c
> > @@ -141,66 +141,62 @@ void virSystemdHasLogindResetCachedValue(void)
> > }
> >
> >
> > -/* -2 = machine1 is not supported on this machine
> > - * -1 = error
> > - *  0 = machine1 is available
> > +/**
> > + * virSystemdHasService:
> > + *
> > + * Check whether a DBus @service is enabled and either the service itself or
> > + * systemd1 service is registered. If @requireSystemd == true, the systemd1
> > + * service has to be registered even if @service is registered.
> > + *
> > + * Returns
> > + *   -2 when service is not supported on this machine
> > + *   -1 on error
> > + *    0 when service is available
> >  */
> > -int
> > -virSystemdHasMachined(void)
> > +static int
> > +virSystemdHasService(const char *service,
> > +                     bool requireSystemd,
> > +                     int *cached)
> > {
> >     int ret;
> >     int val;
> >
> > -    val = g_atomic_int_get(&virSystemdHasMachinedCachedValue);
> > +    val = g_atomic_int_get(cached);
> >     if (val != -1)
> >         return val;
> >
> > -    if ((ret = virGDBusIsServiceEnabled("org.freedesktop.machine1")) < 0) {
> > +    if ((ret = virGDBusIsServiceEnabled(service)) < 0) {
> >         if (ret == -2)
> > -            g_atomic_int_set(&virSystemdHasMachinedCachedValue, -2);
> > +            g_atomic_int_set(cached, -2);
> >         return ret;
> >     }
> >
> > -    if ((ret = virGDBusIsServiceRegistered("org.freedesktop.systemd1")) == -1)
> > +    if ((ret = virGDBusIsServiceRegistered(service)) == -1)
> >         return ret;
>
> From the 'refactor' point of view, this adds the extra check whether
> machine1 is also registered. So the bool should rather be
> 'skipRunningCheck' or something like that.
>
> From a practical point of view, this seems pointless if we discard the result anyway.
> Admittedly, the terminology I added in:
> commit 12ee0b98d3ddcb2c71cdb2352512153791ebcb7e
>     Check if systemd is running before creating machines
> is a bit confusing. But if "machine1" is "Enabled" (i.e. in
> "ListActivatableNames"), that could have meant that there is systemd
> installed but the system was not booted with it.
>
> If it returns -1, the following (unreachable) call would likely return
> -1 as well.
> If it returns 0, we don't need to check whether systemd1 is registered,
> because we already know machined is running.
> If it returns -2, we did not really learn anything new.
>
> > -    g_atomic_int_set(&virSystemdHasMachinedCachedValue, ret);
> > +
> > +    if (requireSystemd || ret == -2) {
> > +        if ((ret = virGDBusIsServiceRegistered("org.freedesktop.systemd1")) == -1)
> > +            return ret;
> > +    }
> > +
> > +    g_atomic_int_set(cached, ret);
> >     return ret;
> > }
> >
> > +
> > int
> > -virSystemdHasLogind(void)
> > +virSystemdHasMachined(void)
> > {
> > -    int ret;
> > -    int val;
> > -
> > -    val = g_atomic_int_get(&virSystemdHasLogindCachedValue);
> > -    if (val != -1)
> > -        return val;
> > -
> > -    ret = virGDBusIsServiceEnabled("org.freedesktop.login1");
> > -    if (ret < 0) {
> > -        if (ret == -2)
> > -            g_atomic_int_set(&virSystemdHasLogindCachedValue, -2);
> > -        return ret;
> > -    }
> > -
> > -    /*
> > -     * Want to use logind if:
> > -     *   - logind is already running
> > -     * Or
> > -     *   - logind is not running, but this is a systemd host
> > -     *     (rely on dbus activation)
> > -     */
> > -    if ((ret = virGDBusIsServiceRegistered("org.freedesktop.login1")) == -1)
> > -        return ret;
>
> Is there a possibility of logind running on a host not booted with
> systemd? If not, this check is also pointless with the fallback below.

Sure, yes, I believe so.

> (Same question applies to resolved, but is not relevant to this patch)

But I'm not sure about resolved.  From another point of view, someone
could just write that (or maybe use the systemd one) as a drop-in
replacement that does not need systemd.  I'd rather check for something
that we think "can't ever be false" than coming back to the issue later
on.

Martin

> Jano
>
> > +    return virSystemdHasService("org.freedesktop.machine1", true,
> > +                                &virSystemdHasMachinedCachedValue);
> > +}
> >
> > -    if (ret == -2) {
> > -        if ((ret = virGDBusIsServiceRegistered("org.freedesktop.systemd1")) == -1)
> > -            return ret;
> > -    }
> >
> > -    g_atomic_int_set(&virSystemdHasLogindCachedValue, ret);
> > -    return ret;
> > +int
> > +virSystemdHasLogind(void)
> > +{
> > +    return virSystemdHasService("org.freedesktop.login1", false,
> > +                                &virSystemdHasLogindCachedValue);
> > }
> >
> >
> > --
> > 2.43.0
> > _______________________________________________
> > Devel mailing list -- devel@xxxxxxxxxxxxxxxxx
> > To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx



> _______________________________________________
> Devel mailing list -- devel@xxxxxxxxxxxxxxxxx
> To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx

Attachment: signature.asc
Description: PGP signature

_______________________________________________
Devel mailing list -- devel@xxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx