Re: [PATCH] build: Embed dbus_message_unref in virdbus

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


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

Attachment: signature.asc
Description: Digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]