On Fri, Feb 02, 2024 at 13:57:37 +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.
I see, so we actually do not care about systemd running, we just need
machined service to be either running or started by socket activation,
that is the same thing we do for other systemd services.
> > - 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.
Yes, it is. For example Gentoo provides a separate logind daemon for
openrc based installations.
> (Same question applies to resolved, but is not relevant to this patch)
I don't know, but I guess it could exist similarly to logind.
Jirka
_______________________________________________
Devel mailing list -- devel@xxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx
