[libvirt PATCH v2 1/7] util: Unify virSystemdHas{Machined,Logind}

Jiri Denemark posted 7 patches 1 year, 11 months ago
[libvirt PATCH v2 1/7] util: Unify virSystemdHas{Machined,Logind}
Posted by Jiri Denemark 1 year, 11 months ago
When checking for machined we do not really care whether systemd itself
is running, we just need machined to be either running or socket
activated by systemd. That is, exactly the same we do for logind.

Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
---
 src/util/virsystemd.c | 74 ++++++++++++++++++++-----------------------
 1 file changed, 34 insertions(+), 40 deletions(-)

diff --git a/src/util/virsystemd.c b/src/util/virsystemd.c
index cd4de0eef8..36820e81fe 100644
--- a/src/util/virsystemd.c
+++ b/src/util/virsystemd.c
@@ -141,57 +141,35 @@ 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 is
+ * running or will be started on demand by a running systemd1 service.
+ *
+ * Returns
+ *   -2 when service is not supported on this machine
+ *   -1 on error
+ *    0 when service is available
  */
-int
-virSystemdHasMachined(void)
-{
-    int ret;
-    int val;
-
-    val = g_atomic_int_get(&virSystemdHasMachinedCachedValue);
-    if (val != -1)
-        return val;
-
-    if ((ret = virGDBusIsServiceEnabled("org.freedesktop.machine1")) < 0) {
-        if (ret == -2)
-            g_atomic_int_set(&virSystemdHasMachinedCachedValue, -2);
-        return ret;
-    }
-
-    if ((ret = virGDBusIsServiceRegistered("org.freedesktop.systemd1")) == -1)
-        return ret;
-    g_atomic_int_set(&virSystemdHasMachinedCachedValue, ret);
-    return ret;
-}
-
-int
-virSystemdHasLogind(void)
+static int
+virSystemdHasService(const char *service,
+                     int *cached)
 {
     int ret;
     int val;
 
-    val = g_atomic_int_get(&virSystemdHasLogindCachedValue);
+    val = g_atomic_int_get(cached);
     if (val != -1)
         return val;
 
-    ret = virGDBusIsServiceEnabled("org.freedesktop.login1");
-    if (ret < 0) {
+    if ((ret = virGDBusIsServiceEnabled(service)) < 0) {
         if (ret == -2)
-            g_atomic_int_set(&virSystemdHasLogindCachedValue, -2);
+            g_atomic_int_set(cached, -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)
+    if ((ret = virGDBusIsServiceRegistered(service)) == -1)
         return ret;
 
     if (ret == -2) {
@@ -199,11 +177,27 @@ virSystemdHasLogind(void)
             return ret;
     }
 
-    g_atomic_int_set(&virSystemdHasLogindCachedValue, ret);
+    g_atomic_int_set(cached, ret);
     return ret;
 }
 
 
+int
+virSystemdHasMachined(void)
+{
+    return virSystemdHasService("org.freedesktop.machine1",
+                                &virSystemdHasMachinedCachedValue);
+}
+
+
+int
+virSystemdHasLogind(void)
+{
+    return virSystemdHasService("org.freedesktop.login1",
+                                &virSystemdHasLogindCachedValue);
+}
+
+
 /**
  * virSystemdGetMachineByPID:
  * @conn: dbus connection
-- 
2.43.0
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
Re: [libvirt PATCH v2 1/7] util: Unify virSystemdHas{Machined,Logind}
Posted by Ján Tomko 1 year, 11 months ago
On a Monday in 2024, Jiri Denemark wrote:
>When checking for machined we do not really care whether systemd itself
>is running, we just need machined to be either running or socket
>activated by systemd. That is, exactly the same we do for logind.
>
>Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
>---
> src/util/virsystemd.c | 74 ++++++++++++++++++++-----------------------
> 1 file changed, 34 insertions(+), 40 deletions(-)
>

Reviewed-by: Ján Tomko <jtomko@redhat.com>

Jano
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
Re: [libvirt PATCH v2 1/7] util: Unify virSystemdHas{Machined,Logind}
Posted by Daniel P. Berrangé 1 year, 11 months ago
On Mon, Feb 05, 2024 at 03:14:08PM +0100, Jiri Denemark wrote:
> When checking for machined we do not really care whether systemd itself
> is running, we just need machined to be either running or socket
> activated by systemd. That is, exactly the same we do for logind.

That's not right. We very much *do* care whether systemd is
running.

If systemd-machined is installed on the host but the OS is
booted into sysvinit, then DBus will report that machined
can be activated, but if it is activated then it certainly
won't actually work.

> Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
> ---
>  src/util/virsystemd.c | 74 ++++++++++++++++++++-----------------------
>  1 file changed, 34 insertions(+), 40 deletions(-)


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
Re: Re: [libvirt PATCH v2 1/7] util: Unify virSystemdHas{Machined,Logind}
Posted by Jiri Denemark 1 year, 11 months ago
On Mon, Feb 05, 2024 at 14:42:09 +0000, Daniel P. Berrangé wrote:
> On Mon, Feb 05, 2024 at 03:14:08PM +0100, Jiri Denemark wrote:
> > When checking for machined we do not really care whether systemd itself
> > is running, we just need machined to be either running or socket
> > activated by systemd. That is, exactly the same we do for logind.
> 
> That's not right. We very much *do* care whether systemd is
> running.
> 
> If systemd-machined is installed on the host but the OS is
> booted into sysvinit, then DBus will report that machined
> can be activated, but if it is activated then it certainly
> won't actually work.

I guess I didn't explain myself correctly, but this just changes our
machind service check to do the same we do for logind. That is, machine1
service must be enabled (listed by ListActivatableNames). If it is, it
must already be running (reported by ListNames) or systemd must be
running.

Are you saying we should properly handle situations when a service we
want to talk to is already running, but cannot work because systemd is
not running? I would expect the service never be running in such case as
it would just fail to start without systemd.

Jirka
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
Re: Re: [libvirt PATCH v2 1/7] util: Unify virSystemdHas{Machined,Logind}
Posted by Daniel P. Berrangé 1 year, 11 months ago
On Mon, Feb 05, 2024 at 04:02:55PM +0100, Jiri Denemark wrote:
> On Mon, Feb 05, 2024 at 14:42:09 +0000, Daniel P. Berrangé wrote:
> > On Mon, Feb 05, 2024 at 03:14:08PM +0100, Jiri Denemark wrote:
> > > When checking for machined we do not really care whether systemd itself
> > > is running, we just need machined to be either running or socket
> > > activated by systemd. That is, exactly the same we do for logind.
> > 
> > That's not right. We very much *do* care whether systemd is
> > running.
> > 
> > If systemd-machined is installed on the host but the OS is
> > booted into sysvinit, then DBus will report that machined
> > can be activated, but if it is activated then it certainly
> > won't actually work.
> 
> I guess I didn't explain myself correctly, but this just changes our
> machind service check to do the same we do for logind. That is, machine1
> service must be enabled (listed by ListActivatableNames). If it is, it
> must already be running (reported by ListNames) or systemd must be
> running.
> 
> Are you saying we should properly handle situations when a service we
> want to talk to is already running, but cannot work because systemd is
> not running? I would expect the service never be running in such case as
> it would just fail to start without systemd.

Hmm, I guess that's safe enough actually.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org