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

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(a)redhat.com&gt; >>--- >>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.