Re: [PATCH] systemd: fix build without dbus

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx>
---
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.

Martin

Attachment: pgpuWOtQPpZfZ.pgp
Description: PGP signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]