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