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

Martin

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]