To reduce code duplication both function now use a common
virSystemdHasService helper.

Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
---
 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;
-    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;
+    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@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

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@redhat.com>
>---
> 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@lists.libvirt.org
>To unsubscribe send an email to devel-leave@lists.libvirt.org
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

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@redhat.com>
> > ---
> > 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@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

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@redhat.com>
>>---
>> 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@lists.libvirt.org
>>To unsubscribe send an email to devel-leave@lists.libvirt.org



>_______________________________________________
>Devel mailing list -- devel@lists.libvirt.org
>To unsubscribe send an email to devel-leave@lists.libvirt.org

_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org