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@xxxxxxxxxxxxxxxxxx>
> >> *
> >> * 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.
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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list