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

[Ján Tomko <jtomko@xxxxxxxxxx>]

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.

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

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

Attachment: signature.asc
Description: PGP signature

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