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

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(a)redhat.com&gt; > --- > 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.

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