[libvirt] [PATCH] build: Embed dbus_message_unref in virdbus

[Martin Kletzander]

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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140414/eb142a3e/attachment-0001.sig>