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.b...@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? Martin
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
