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