[libvirt] [PATCH] build: Embed dbus_message_unref in virdbus

[Daniel P. Berrange]

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
> 
> +    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.


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