[libvirt] [PATCH] systemd: fix build without dbus
Martin Kletzander
mkletzan at redhat.com
Mon Jan 19 13:50:19 UTC 2015
On Mon, Jan 19, 2015 at 01:28:15PM +0000, Daniel P. Berrange wrote:
>On Mon, Jan 19, 2015 at 02:13:12PM +0100, Martin Kletzander wrote:
>> On Mon, Jan 19, 2015 at 12:34:52PM +0000, Daniel P. Berrange wrote:
>> >The virDBusMethodCall method has a DBusError as one of its
>> >parameters. If the caller wants to pass a non-NULL value
>> >for this, it immediately makes the calling code require
>> >DBus at build time. This has led to breakage of non-DBus
>> >builds several times. It is desirable that only the virdbus.c
>> >file should need WITH_DBUS conditionals, so we must ideally
>> >remove the DBusError parameter from the method.
>> >
>> >We can't simply raise a libvirt error, since the whole point
>> >of this parameter is to give the callers a way to check if
>> >the error is one they want to ignore, without having the logs
>> >polluted with an error message. So, we add a virErrorPtr
>> >parameter which the caller can then either ignore or raise
>> >using virSetError.
>> >
>> >Signed-off-by: Daniel P. Berrange <berrange at redhat.com>
>> >---
>> >src/util/virdbus.c | 31 +++++++++++++++++++------------
>> >src/util/virdbus.h | 4 ++--
>> >src/util/virfirewall.c | 34 ++++++++++------------------------
>> >src/util/virsystemd.c | 15 +++++++--------
>> >4 files changed, 38 insertions(+), 46 deletions(-)
>> >
>> >This is a build-breaker fix, but I'm not pushing since the
>> >fix is too complicated to go in without some review.
>> >
>> [...]
>> >diff --git a/src/util/virdbus.h b/src/util/virdbus.h
>> >index d0c7de2..e2b8d2b 100644
>> >--- a/src/util/virdbus.h
>> >+++ b/src/util/virdbus.h
>> >@@ -28,7 +28,7 @@
>> ># else
>> ># define DBusConnection void
>> ># define DBusMessage void
>> >-# define DBusError void
>>
>> Using virError instead of DBusError is fine, but
>>
>> >+# define dbus_message_unref(m) do {} while (0)
>>
>> ewww, can't we just cleanly separate DBus code from libvirt code
>> instead of adding more of these?
>
>This is required because we removed the WITH_DBUS conditional
>from virsystemd. To eliminate this would require that we remove
>DBusMessage from the virdbus.h header files too. That is doable
>but a bigger job and not really needed to fix this current build
>problem, so I think this is appropriate fix for this current
>patch.
>
>>
>> >diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c
>> >index b536912..a01b703 100644
>> >--- a/src/util/virfirewall.c
>> >+++ b/src/util/virfirewall.c
>> [...]
>> >@@ -789,10 +777,13 @@ virFirewallApplyRuleFirewallD(virFirewallRulePtr rule,
>> > "sa&s",
>> > ipv,
>> > (int)rule->argsLen,
>> >- rule->args) < 0)
>> >+ rule->args) < 0) {
>> >+ VIR_ERROR("Here fail");
>>
>> Leftover from debugging?
>
>Opps, yes.
>
>> > goto cleanup;
>> >+ }
>> >
>> >- if (dbus_error_is_set(&error)) {
>> >+ VIR_ERROR("Error %d: %s\n", error.level, error.message);
>> >+ if (error.level == VIR_ERR_ERROR) {
>> > /*
>> > * As of firewalld-0.3.9.3-1.fc20.noarch the name and
>> > * message fields in the error look like
>> [...]
>> >@@ -862,12 +850,10 @@ virFirewallApplyRule(virFirewallPtr firewall,
>> > if (virFirewallApplyRuleDirect(rule, ignoreErrors, &output) < 0)
>> > return -1;
>> > break;
>> >-#if WITH_DBUS
>> > case VIR_FIREWALL_BACKEND_FIREWALLD:
>> > if (virFirewallApplyRuleFirewallD(rule, ignoreErrors, &output) < 0)
>>
>> Another thing that's pre-existing, but we are adding more and more of
>> them. I don't like that libvirt is still using systemd stuff even if
>> configured not to (--without-systemd).
>
>The --without-systemd flag is about use of the libsystemd-daemon.so
>library, which is used for daemon startup notification.
>
>Everything else related to systemd is just plain DBus API calls, so
>it is not appropriate to tie them to the --without-systemd flag. All
>DBus calls are dealt with by runtime checks for the service, rather
>than compile time checks for a lbirary.
>
>> >diff --git a/src/util/virsystemd.c b/src/util/virsystemd.c
>> >index 3eea5c2..0b71b26 100644
>> >--- a/src/util/virsystemd.c
>> >+++ b/src/util/virsystemd.c
>> >@@ -252,8 +252,8 @@ int virSystemdCreateMachine(const char *name,
>> >
>> > VIR_DEBUG("Attempting to create machine via systemd");
>> > if (virAtomicIntGet(&hasCreateWithNetwork)) {
>> >- DBusError error;
>> >- dbus_error_init(&error);
>> >+ virError error;
>> >+ memset(&error, 0, sizeof(error));
>> >
>> > if (virDBusCallMethod(conn,
>> > NULL,
>> >@@ -280,21 +280,20 @@ int virSystemdCreateMachine(const char *name,
>> > "Before", "as", 1, "libvirt-guests.service") < 0)
>> > goto cleanup;
>> >
>> >- if (dbus_error_is_set(&error)) {
>> >+ if (error.level == VIR_ERR_ERROR) {
>> > if (STREQ_NULLABLE("org.freedesktop.DBus.Error.UnknownMethod",
>> >- error.name)) {
>> >+ error.str1)) {
>> > VIR_INFO("CreateMachineWithNetwork isn't supported, switching "
>> > "to legacy CreateMachine method for systemd-machined");
>> >- dbus_error_free(&error);
>> >+ virResetError(&error);
>> > virAtomicIntSet(&hasCreateWithNetwork, 0);
>> > /* Could re-structure without Using goto, but this
>> > * avoids another atomic read which would trigger
>> > * another memory barrier */
>> > goto fallback;
>> > }
>> >- virReportError(VIR_ERR_DBUS_SERVICE,
>> >- _("CreateMachineWithNetwork: %s"),
>> >- error.message ? error.message : _("unknown error"));
>> >+ virSetError(&error);
>> >+ virResetError(&error);
>> > goto cleanup;
>> > }
>> > } else {
>> >--
>>
>> This is pretty hackish IMHO. Since
>> "org.freedesktop.DBus.Error.UnknownMethod" isn't specific for systemd,
>> we can make virDBusCall() report whether unknown method was called or
>> not and then decide what to do with it two layers up in the stack. It
>> also simplifies virSystemdCreateMachine() a lot.
>
>The "org.freedesktop.DBus.Error.UnknownMethod" method is a standard error
>code that is to be raised by DBus services, when the method requested does
>not exist. This is exactly the scenario we are dealing with here. We tried
>to run the CreateMachineWithNetwork method and it doesn't exist, so looking
>for the UnknownMethod error name is the correct handling for this scenario
>and how DBus error handling is intended to work.
>
Yes, I know, my idea was rather something like the following, although
that does not have the DBusError -> virError change done in itself:
diff --git a/src/util/virdbus.c b/src/util/virdbus.c
index d9665c1..1f232a6 100644
--- a/src/util/virdbus.c
+++ b/src/util/virdbus.c
@@ -1,7 +1,7 @@
/*
* virdbus.c: helper for using DBus
*
- * Copyright (C) 2012-2014 Red Hat, Inc.
+ * Copyright (C) 2012-2015 Red Hat, Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -1522,8 +1522,8 @@ static int
virDBusCall(DBusConnection *conn,
DBusMessage *call,
DBusMessage **replyout,
- DBusError *error)
-
+ DBusError *error,
+ bool missing_ok)
{
DBusMessage *reply = NULL;
DBusError localerror;
@@ -1553,6 +1553,11 @@ virDBusCall(DBusConnection *conn,
error ? error->message : localerror.message);
if (error) {
ret = 0;
+ } else if (missing_ok &&
+ dbus_error_is_set(&error) &&
+ STREQ_NULLABLE("org.freedesktop.DBus.Error.UnknownMethod",
+ error.name)) {
+ ret = -2;
} else {
virReportError(VIR_ERR_DBUS_SERVICE, _("%s: %s"), member,
localerror.message ? localerror.message : _("unknown error"));
@@ -1617,6 +1622,7 @@ virDBusCall(DBusConnection *conn,
int virDBusCallMethod(DBusConnection *conn,
DBusMessage **replyout,
DBusError *error,
+ bool missing_ok,
const char *destination,
const char *path,
const char *iface,
@@ -1634,9 +1640,10 @@ int virDBusCallMethod(DBusConnection *conn,
if (ret < 0)
goto cleanup;
- ret = -1;
+ ret = virDBusCall(conn, call, replyout, error, missing_ok);
- ret = virDBusCall(conn, call, replyout, error);
+ if (ret == -2)
+ VIR_INFO("DBus method '%s' isn't supported", member);
cleanup:
if (call)
--
Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150119/5cf41f4f/attachment-0001.sig>
More information about the libvir-list
mailing list