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

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(a)linux.vnet.ibm.com&gt; > * > * 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.