9:47 a.m.
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
9:47 a.m.
New subject: [libvirt] [PATCHv2 1/2] Split out most of virDBusIsServiceEnabled
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
1:55 p.m.
New subject: [libvirt] [PATCHv2 1/2] Split out most of virDBusIsServiceEnabled
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
2:21 a.m.
New subject: [libvirt] [PATCHv2 1/2] Split out most of virDBusIsServiceEnabled
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
9:47 a.m.
New subject: [libvirt] [PATCHv2 2/2] Check if systemd is running before creating machines
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
1:57 p.m.
New subject: [libvirt] [PATCHv2 2/2] Check if systemd is running before creating machines
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
2:21 a.m.
New subject: [libvirt] [PATCHv2 2/2] Check if systemd is running before creating machines
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
3916
days inactive
3917
days old
6 comments
2 participants
participants (2)
-
Eric Blake -
Ján Tomko