[libvirt] [PATCHv2 0/2] Check if systemd is the init before creating machines
v1: https://www.redhat.com/archives/libvir-list/2014-February/msg01662.html v2: check if systemd1 is registered in ListNames instead of reading /proc/1/comm actually pass 'make check' in all three cases: * no systemd * systemd installed * systemd running Ján Tomko (2): Split out most of virDBusIsServiceEnabled Check if systemd is running before creating machines src/util/virdbus.c | 47 +++++++++++++++++++++++++++++++++++++---------- src/util/virdbus.h | 1 + src/util/virsystemd.c | 6 ++++++ tests/virsystemdmock.c | 24 +++++++++++++++++++++++- tests/virsystemdtest.c | 37 +++++++++++++++++++++++++++++++++++++ 5 files changed, 104 insertions(+), 11 deletions(-) -- 1.8.3.2
Introduce virDBusIsServiceInList which can be used to call other methods for listing services (ListNames), not just ListActivatableNames. No functional change. --- src/util/virdbus.c | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/src/util/virdbus.c b/src/util/virdbus.c index a6232b7..3c9416e 100644 --- a/src/util/virdbus.c +++ b/src/util/virdbus.c @@ -1256,13 +1256,7 @@ int virDBusMessageRead(DBusMessage *msg, return ret; } -/** - * virDBusIsServiceEnabled: - * @name: service name - * - * Retruns 0 if service is available, -1 on fatal error, or -2 if service is not available - */ -int virDBusIsServiceEnabled(const char *name) +static int virDBusIsServiceInList(const char *listMethod, const char *name) { DBusConnection *conn; DBusMessage *reply = NULL; @@ -1280,7 +1274,7 @@ int virDBusIsServiceEnabled(const char *name) "org.freedesktop.DBus", "/org/freedesktop/DBus", "org.freedesktop.DBus", - "ListActivatableNames", + listMethod, NULL) < 0) return ret; @@ -1305,13 +1299,25 @@ int virDBusIsServiceEnabled(const char *name) } } - VIR_DEBUG("Service %s is %s", name, ret ? "unavailable" : "available"); - cleanup: dbus_message_unref(reply); return ret; } +/** + * virDBusIsServiceEnabled: + * @name: service name + * + * Retruns 0 if service is available, -1 on fatal error, or -2 if service is not available + */ +int virDBusIsServiceEnabled(const char *name) +{ + int ret = virDBusIsServiceInList("ListActivatableNames", name); + + VIR_DEBUG("Service %s is %s", name, ret ? "unavailable" : "available"); + + return ret; +} #else /* ! WITH_DBUS */ void virDBusSetSharedBus(bool shared ATTRIBUTE_UNUSED) -- 1.8.3.2
On 03/03/2014 08:47 AM, Ján Tomko wrote:
Introduce virDBusIsServiceInList which can be used to call other methods for listing services (ListNames), not just ListActivatableNames.
Nice refactoring.
No functional change. --- src/util/virdbus.c | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-)
-/** - * virDBusIsServiceEnabled: - * @name: service name - * - * Retruns 0 if service is available, -1 on fatal error, or -2 if service is not available
Hmm, the "Retruns" typo...
- */ -int virDBusIsServiceEnabled(const char *name) +static int virDBusIsServiceInList(const char *listMethod, const char *name)
Here's your chance to use the two-line formatting: static int virDBusIs...(...)
+/** + * virDBusIsServiceEnabled: + * @name: service name + * + * Retruns 0 if service is available, -1 on fatal error, or -2 if service is not available
...merely moved down. Please fix it while touching here :)
+ */ +int virDBusIsServiceEnabled(const char *name)
Two-line formatting might be nice. My findings are cosmetic, so ACK whether you address them or not. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 03/03/2014 08:55 PM, Eric Blake wrote:
On 03/03/2014 08:47 AM, Ján Tomko wrote:
Introduce virDBusIsServiceInList which can be used to call other methods for listing services (ListNames), not just ListActivatableNames.
Nice refactoring.
No functional change. --- src/util/virdbus.c | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-)
+/** + * virDBusIsServiceEnabled: + * @name: service name + * + * Retruns 0 if service is available, -1 on fatal error, or -2 if service is not available
...merely moved down. Please fix it while touching here :)
I've fixed the typo...
+ */ +int virDBusIsServiceEnabled(const char *name)
Two-line formatting might be nice.
... but kept the single-line formatting since most of the file uses it.
My findings are cosmetic, so ACK whether you address them or not.
Thanks, pushed. Jan
If systemd is installed, but not the init system, systemd-machined fails with an unhelpful error message: Launch helper exited with unknown return code 1 Currently we only check if the "machine1" service is available (in ListActivatableNames). Also check if "systemd1" service is registered with DBus (ListNames). This fixes https://bugs.gentoo.org/show_bug.cgi?id=493246#c22 --- src/util/virdbus.c | 21 +++++++++++++++++++++ src/util/virdbus.h | 1 + src/util/virsystemd.c | 6 ++++++ tests/virsystemdmock.c | 24 +++++++++++++++++++++++- tests/virsystemdtest.c | 37 +++++++++++++++++++++++++++++++++++++ 5 files changed, 88 insertions(+), 1 deletion(-) diff --git a/src/util/virdbus.c b/src/util/virdbus.c index 3c9416e..d15e56e 100644 --- a/src/util/virdbus.c +++ b/src/util/virdbus.c @@ -1319,6 +1319,21 @@ int virDBusIsServiceEnabled(const char *name) return ret; } +/** + * virDBusIsServiceRegistered + * @name: service name + * + * Retruns 0 if service is registered, -1 on fatal error, or -2 if service is not registered + */ +int virDBusIsServiceRegistered(const char *name) +{ + int ret = virDBusIsServiceInList("ListNames", name); + + VIR_DEBUG("Service %s is %sregistered", name, ret ? "not " : ""); + + return ret; +} + #else /* ! WITH_DBUS */ void virDBusSetSharedBus(bool shared ATTRIBUTE_UNUSED) { @@ -1397,4 +1412,10 @@ int virDBusIsServiceEnabled(const char *name ATTRIBUTE_UNUSED) return -2; } +int virDBusIsServiceRegistered(const char *name ATTRIBUTE_UNUSED) +{ + VIR_DEBUG("DBus support not compiled into this binary"); + return -2; +} + #endif /* ! WITH_DBUS */ diff --git a/src/util/virdbus.h b/src/util/virdbus.h index 979c566..1ca8641 100644 --- a/src/util/virdbus.h +++ b/src/util/virdbus.h @@ -49,4 +49,5 @@ int virDBusMessageRead(DBusMessage *msg, const char *types, ...); int virDBusIsServiceEnabled(const char *name); +int virDBusIsServiceRegistered(const char *name); #endif /* __VIR_DBUS_H__ */ diff --git a/src/util/virsystemd.c b/src/util/virsystemd.c index 8adf209..d263c58 100644 --- a/src/util/virsystemd.c +++ b/src/util/virsystemd.c @@ -173,6 +173,9 @@ int virSystemdCreateMachine(const char *name, if (ret < 0) return ret; + if ((ret = virDBusIsServiceRegistered("org.freedesktop.systemd1")) < 0) + return ret; + if (!(conn = virDBusGetSystemBus())) return -1; @@ -274,6 +277,9 @@ int virSystemdTerminateMachine(const char *name, if (ret < 0) return ret; + if ((ret = virDBusIsServiceRegistered("org.freedesktop.systemd1")) < 0) + return ret; + if (!(conn = virDBusGetSystemBus())) return -1; diff --git a/tests/virsystemdmock.c b/tests/virsystemdmock.c index 0295231..b4fcf6e 100644 --- a/tests/virsystemdmock.c +++ b/tests/virsystemdmock.c @@ -66,6 +66,7 @@ DBusMessage *dbus_connection_send_with_reply_and_block(DBusConnection *connectio { DBusMessage *reply = NULL; const char *service = dbus_message_get_destination(message); + const char *member = dbus_message_get_member(message); if (STREQ(service, "org.freedesktop.machine1")) { if (getenv("FAIL_BAD_SERVICE")) { @@ -82,7 +83,8 @@ DBusMessage *dbus_connection_send_with_reply_and_block(DBusConnection *connectio } else { reply = dbus_message_new(DBUS_MESSAGE_TYPE_METHOD_RETURN); } - } else if (STREQ(service, "org.freedesktop.DBus")) { + } else if (STREQ(service, "org.freedesktop.DBus") && + STREQ(member, "ListActivatableNames")) { const char *svc1 = "org.foo.bar.wizz"; const char *svc2 = "org.freedesktop.machine1"; DBusMessageIter iter, sub; @@ -101,6 +103,26 @@ DBusMessage *dbus_connection_send_with_reply_and_block(DBusConnection *connectio &svc2)) goto error; dbus_message_iter_close_container(&iter, &sub); + } else if (STREQ(service, "org.freedesktop.DBus") && + STREQ(member, "ListNames")) { + const char *svc1 = "org.foo.bar.wizz"; + const char *svc2 = "org.freedesktop.systemd1"; + DBusMessageIter iter, sub; + reply = dbus_message_new(DBUS_MESSAGE_TYPE_METHOD_RETURN); + dbus_message_iter_init_append(reply, &iter); + dbus_message_iter_open_container(&iter, DBUS_TYPE_ARRAY, + "s", &sub); + + if (!dbus_message_iter_append_basic(&sub, + DBUS_TYPE_STRING, + &svc1)) + goto error; + if ((!getenv("FAIL_NO_SERVICE") && !getenv("FAIL_NOT_REGISTERED")) && + !dbus_message_iter_append_basic(&sub, + DBUS_TYPE_STRING, + &svc2)) + goto error; + dbus_message_iter_close_container(&iter, &sub); } else { reply = dbus_message_new(DBUS_MESSAGE_TYPE_METHOD_RETURN); } diff --git a/tests/virsystemdtest.c b/tests/virsystemdtest.c index 9f6fc83..9752d12 100644 --- a/tests/virsystemdtest.c +++ b/tests/virsystemdtest.c @@ -135,6 +135,40 @@ static int testCreateNoSystemd(const void *opaque ATTRIBUTE_UNUSED) return 0; } +static int testCreateSystemdNotRunning(const void *opaque ATTRIBUTE_UNUSED) +{ + unsigned char uuid[VIR_UUID_BUFLEN] = { + 1, 1, 1, 1, + 2, 2, 2, 2, + 3, 3, 3, 3, + 4, 4, 4, 4 + }; + int rv; + + setenv("FAIL_NOT_REGISTERED", "1", 1); + + if ((rv = virSystemdCreateMachine("demo", + "qemu", + true, + uuid, + NULL, + 123, + false, + NULL)) == 0) { + unsetenv("FAIL_NOT_REGISTERED"); + fprintf(stderr, "%s", "Unexpected create machine success\n"); + return -1; + } + unsetenv("FAIL_NOT_REGISTERED"); + + if (rv != -2) { + fprintf(stderr, "%s", "Unexpected create machine error\n"); + return -1; + } + + return 0; +} + static int testCreateBadSystemd(const void *opaque ATTRIBUTE_UNUSED) { unsigned char uuid[VIR_UUID_BUFLEN] = { @@ -216,6 +250,9 @@ mymain(void) ret = -1; if (virtTestRun("Test create no systemd ", testCreateNoSystemd, NULL) < 0) ret = -1; + if (virtTestRun("Test create systemd not running ", + testCreateSystemdNotRunning, NULL) < 0) + ret = -1; if (virtTestRun("Test create bad systemd ", testCreateBadSystemd, NULL) < 0) ret = -1; -- 1.8.3.2
On 03/03/2014 08:47 AM, Ján Tomko wrote:
If systemd is installed, but not the init system,
Took me a couple reads to understand this. It would be easier with s/not/is not/
systemd-machined fails with an unhelpful error message: Launch helper exited with unknown return code 1
Currently we only check if the "machine1" service is available (in ListActivatableNames). Also check if "systemd1" service is registered with DBus (ListNames).
This fixes https://bugs.gentoo.org/show_bug.cgi?id=493246#c22 ---
ACK.
+/** + * virDBusIsServiceRegistered + * @name: service name + * + * Retruns 0 if service is registered, -1 on fatal error, or -2 if service is not registered + */ +int virDBusIsServiceRegistered(const char *name)
2 lines, if desired.
+{ + int ret = virDBusIsServiceInList("ListNames", name); + + VIR_DEBUG("Service %s is %sregistered", name, ret ? "not " : "");
Translation nightmare. But this string isn't marked for translation, so it's okay to hard-code English grammar into the ?:. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 03/03/2014 08:57 PM, Eric Blake wrote:
On 03/03/2014 08:47 AM, Ján Tomko wrote:
If systemd is installed, but not the init system,
Took me a couple reads to understand this. It would be easier with s/not/is not/
systemd-machined fails with an unhelpful error message: Launch helper exited with unknown return code 1
Currently we only check if the "machine1" service is available (in ListActivatableNames). Also check if "systemd1" service is registered with DBus (ListNames).
This fixes https://bugs.gentoo.org/show_bug.cgi?id=493246#c22 ---
ACK.
+{ + int ret = virDBusIsServiceInList("ListNames", name); + + VIR_DEBUG("Service %s is %sregistered", name, ret ? "not " : "");
Translation nightmare. But this string isn't marked for translation, so it's okay to hard-code English grammar into the ?:.
I've changed it to: VIR_DEBUG("Service %s is %s", name, ret ? "not registered" : "registered"); to make it easier to read and pushed it with the amended commit message. Jan
participants (2)
-
Eric Blake -
Ján Tomko