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)

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(a)lists.libvirt.org >To unsubscribe send an email to devel-leave(a)lists.libvirt.org

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