[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