[libvirt] [PATCH] build: Embed dbus_message_unref in virdbus
Martin Kletzander
mkletzan at redhat.com
Mon Apr 14 13:39:21 UTC 2014
On Mon, Apr 14, 2014 at 02:29:36PM +0100, Daniel P. Berrange wrote:
>On Mon, Apr 14, 2014 at 02:51:18PM +0200, Martin Kletzander wrote:
>> On Mon, Apr 14, 2014 at 01:33:21PM +0100, Daniel P. Berrange wrote:
>> >On Mon, Apr 14, 2014 at 02:20:11PM +0200, Martin Kletzander wrote:
>> >>On Mon, Apr 14, 2014 at 01:02:02PM +0100, Daniel P. Berrange wrote:
>> >>>On Mon, Apr 14, 2014 at 01:41:18PM +0200, Martin Kletzander wrote:
>> >>>>Adding dbus_message_unref() into virDBusMessageDecodeArgs() makes sure
>> >>>>that the message gets unref'd, thus making one more pure dbus call not
>> >>>>necessary. Even though we are calling a lot of dbus_* functions
>> >>>>outside virdbus (which should be fixed in the future IMHO), this patch
>> >>>>fixes only this one instance because it merely aims to fix a
>> >>>>build-breaker caused by improperly included dbus.h. The message
>> >>>>printed when failing (using --without-dbus) is:
>> >>>
>> >>>
>> >>>>diff --git a/src/util/virdbus.c b/src/util/virdbus.c
>> >>>>index 0cd3858..aef1d34 100644
>> >>>>--- a/src/util/virdbus.c
>> >>>>+++ b/src/util/virdbus.c
>> >>>>@@ -1112,6 +1112,7 @@ int virDBusMessageDecodeArgs(DBusMessage* msg,
>> >>>> }
>> >>>>
>> >>>> ret = virDBusMessageIterDecode(&iter, types, args);
>> >>>>+ dbus_message_unref(msg);
>> >>>>
>> >>>> cleanup:
>> >>>> return ret;
>> >>>
>> >>>NACK, this is basically reverting the change I did previously and
>> >>>will break the firewall patches I have pending.
>> >>>
>> >>
>> >>OK, this makes sense if we do one of the following:
>> >>
>> >>a) Implement a virDBusMessageUnref() which does the same thing if we
>> >> have dbus, which would be no-op otherwise, thus making pure dbus_*
>> >> calls completely wrapped such uses or
>> >>
>> >>b) encapsulate systemd checks for pm-utils in ifdef
>> >> WITH_SYSTEMD_DAEMON like this:
>> >>
>> >>diff --git i/src/util/virnodesuspend.c w/src/util/virnodesuspend.c
>> >>index 59b84ef..20b85b8 100644
>> >>--- i/src/util/virnodesuspend.c
>> >>+++ w/src/util/virnodesuspend.c
>> >>@@ -1,6 +1,7 @@
>> >>/*
>> >> * virnodesuspend.c: Support for suspending a node (host machine)
>> >> *
>> >>+ * Copyright (C) 2014 Red Hat, Inc.
>> >> * Copyright (C) 2011 Srivatsa S. Bhat <srivatsa.bhat at linux.vnet.ibm.com>
>> >> *
>> >> * This library is free software; you can redistribute it and/or
>> >>@@ -269,6 +270,7 @@ virNodeSuspendSupportsTargetPMUtils(unsigned int target, bool *supported)
>> >>}
>> >>#endif
>> >>
>> >>+#ifdef WITH_SYSTEMD_DAEMON
>> >>static int
>> >>virNodeSuspendSupportsTargetSystemd(unsigned int target, bool *supported)
>> >>{
>> >>@@ -292,6 +294,7 @@ virNodeSuspendSupportsTargetSystemd(unsigned int target, bool *supported)
>> >>
>> >> return ret;
>> >>}
>> >>+#endif /* WITH_SYSTEMD_DAEMON */
>> >>
>> >>/**
>> >> * virNodeSuspendSupportsTarget:
>> >>@@ -310,14 +313,23 @@ virNodeSuspendSupportsTargetSystemd(unsigned int target, bool *supported)
>> >>static int
>> >>virNodeSuspendSupportsTarget(unsigned int target, bool *supported)
>> >>{
>> >>- int ret;
>> >>+ int ret = 0;
>> >>+
>> >>+#ifdef WITH_SYSTEMD_DAEMON
>> >>+ if (virNodeSuspendSupportsTargetSystemd(target, supported) == 0)
>> >>+ goto cleanup;
>> >>+#endif
>> >>
>> >>- ret = virNodeSuspendSupportsTargetSystemd(target, supported);
>> >>#ifdef WITH_PM_UTILS
>> >>- if (ret < 0)
>> >>- ret = virNodeSuspendSupportsTargetPMUtils(target, supported);
>> >>+ if (virNodeSuspendSupportsTargetPMUtils(target, supported) == 0)
>> >>+ goto cleanup;
>> >>#endif
>>
>> [1]
>>
>> >>
>> >>+ ret = -1;
>> >>+ virReportSystemError(VIR_ERR_INTERNAL_ERROR, "%s",
>> >>+ _("Unable to get supported suspend types"));
>> >>+
>> >>+ cleanup:
>> >> return ret;
>> >>}
>> >>
>> >>diff --git i/src/util/virsystemd.c w/src/util/virsystemd.c
>> >>index e9ca564..d953f8a 100644
>> >>--- i/src/util/virsystemd.c
>> >>+++ w/src/util/virsystemd.c
>> >>@@ -1,7 +1,7 @@
>> >>/*
>> >> * virsystemd.c: helpers for using systemd APIs
>> >> *
>> >>- * Copyright (C) 2013 Red Hat, Inc.
>> >>+ * Copyright (C) 2013, 2014 Red Hat, Inc.
>> >> *
>> >> * This library is free software; you can redistribute it and/or
>> >> * modify it under the terms of the GNU Lesser General Public
>> >>@@ -326,6 +326,7 @@ virSystemdNotifyStartup(void)
>> >>#endif
>> >>}
>> >>
>> >>+#ifdef WITH_SYSTEMD_DAEMON
>> >>static int
>> >>virSystemdPMSupportTarget(const char *methodName, bool *result)
>> >>{
>> >>@@ -370,6 +371,17 @@ virSystemdPMSupportTarget(const char *methodName, bool *result)
>> >> return ret;
>> >>}
>> >>
>> >>+#else /* ! WITH_SYSTEMD_DAEMON */
>> >>+
>> >>+static int
>> >>+virSystemdPMSupportTarget(const char *methodName ATTRIBUTE_UNUSED,
>> >>+ bool *result ATTRIBUTE_UNUSED)
>> >>+{
>> >>+ return -1;
>> >>+}
>> >>+
>> >>+#endif /* ! WITH_SYSTEMD_DAEMON */
>> >
>> >How about making this method return '0' instead of '-1'. After all,
>> >if we haven't built systemd,then logging it doesn't support the PM
>> >mode being queried. That way we don't need conditionals for the
>> >callers of this method - they'll just do the right thing.
>> >
>>
>> But then virNodeSuspendSupportsTarget() [1] will succeed without
>> querying anything else. Or have I misunderstood and you meant
>> changing virNodeSuspendSupportsTarget() so that it queries everything
>> possible until supported != true?
>
>Hmm, actually I see there's a flaw in virNodeSuspendSupportsTarget in
>the way it deals with fallback. It is not distinguishing between the
>case that systemd login1 servie is not available, from the case where
>checking returns a hard error.
>
>Basically when systemd is compiled out, we need to make it appear that
>login1 is not available.
>
>So IMHO virSystemdCanXXXX needs to have 3 return values
>
> 1: service available, *result gives value
> 0: service not available.
> -1: error querying service
>
>Then virNodeSuspendSupportsTarget, should only fallback to trying the
>pm-utils code when ret == 0.
>
We usually use ret == 0 as complete success, so I'd flip the meaning
of 1 and 0 (if you don't want to use -1 and -2 for different errors as
in other APIs), but otherwise it should fix the problem, so I'm
sending it as a v2 when tested.
Martin
>Then when systemd is compiled out, we can return ret ==0.
>
>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 :|
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140414/38df5a4d/attachment-0001.sig>
More information about the libvir-list
mailing list