[libvirt] [PATCH] systemd: fix build without dbus

[Martin Kletzander]

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?
>
>>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?
>
>>        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).
>
>>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.
>

Not to mention DBus offers introspection of service properties, but I
must admit I have no idea how to make use of it using its C API.

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/6f5add07/attachment-0001.sig>