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 02:24:34PM +0100, Martin Kletzander 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@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.
> >
> 
> 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.

The introspection is primarily intended to non-C language bindings that
need to know what data types to use when encoding/decoding data, since
their languages are typically loosely typed. While you could parse the
introspection to check if a method exists ahead of time, it is really
way overkill for this problem and would require a seriously large amount
of code to deal with it. There's just no win for doing that here.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

--
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]