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

Re: [libvirt] [PATCH 1/2] Introduce virDBusCallMethod & virDBusMessageRead methods



On 07/18/2013 07:27 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange redhat com>
> 
> Doing DBus method calls using libdbus.so is tedious in the
> extreme. systemd developers came up with a nice high level
> API for DBus method calls (sd_bus_call_method). While
> systemd doesn't use libdbus.so, their API design can easily
> be ported to libdbus.so.
> 
> This patch thus introduces methods virDBusCallMethod &
> virDBusMessageRead, which are based on the code used for
> sd_bus_call_method and sd_bus_message_read. This code in
> systemd is under the LGPLv2+, so we're license compatible.
> 
> This code is probably pretty unintelligible unless you are
> familiar with the DBus type system. So I added some API
> docs trying to explain how to use them, as well as test
> cases to validate that I didn't screw up the adaptation
> from the original systemd code.
> 
> Signed-off-by: Daniel P. Berrange <berrange redhat com>

Failed to build.

util/virdbus.c:1119:9: error: implicit declaration of function
'virReportDBusServiceError' [-Werror=implicit-function-declaration]
         virReportDBusServiceError(error.message ? error.message :
"unknown error",
         ^
util/virdbus.c:1119:9: error: nested extern declaration of
'virReportDBusServiceError' [-Werror=nested-externs]

> ---
>  .gitignore               |   1 +
>  src/libvirt_private.syms |   4 +
>  src/util/virdbus.c       | 966 ++++++++++++++++++++++++++++++++++++++++++++++-
>  src/util/virdbus.h       |  11 +
>  src/util/virdbuspriv.h   |  45 +++
>  tests/Makefile.am        |  13 +
>  tests/virdbustest.c      | 389 +++++++++++++++++++
>  7 files changed, 1428 insertions(+), 1 deletion(-)
>  create mode 100644 src/util/virdbuspriv.h
>  create mode 100644 tests/virdbustest.c

> +++ b/.gitignore
> @@ -191,6 +191,7 @@
>  /tests/virbitmaptest
>  /tests/virbuftest
>  /tests/vircgrouptest
> +/tests/virdbustest

Yay - an added test is reassuring (although I didn't actually run it,
due to the compile error).

> +
> +        if (virDBusSignatureLengthInternal(s + 1, true, arrayDepth+1, structDepth, &t) < 0)

Inconsistent spacing around '+'.

> +
> +        while (*p != DBUS_STRUCT_END_CHAR) {
> +            size_t t;
> +
> +            if (virDBusSignatureLengthInternal(p, false, arrayDepth, structDepth+1, &t) < 0)

and again, probably also a long line worth wrapping.


> +
> +            if (virDBusSignatureLengthInternal(p, false, arrayDepth, structDepth+1, &t) < 0)

Recurring theme on '+'; I'll quit pointing them out.

> +
> +/* Ideally, we'd just call ourselves recursively on every
> + * complex type. However, the state of a va_list that is
> + * passed to a function is undefined after that function
> + * returns. This means we need to docode the va_list linearly

d/docode/decode/

> + * in a single stackframe. We hence implement our own
> + * home-grown stack in an array. */

Is it also possible to use va_copy, or is that risking stack explosion
because we'd have to copy the list for each recursion?

> +/**
> + * @conn: a DBus connection

Missing the method name 'virDBusCallMethod:' before the parameters.

> + * @replyout: pointer to receive reply message, or NULL
> + * @destination: bus identifier of the target service
> + * @path: object path of the target service
> + * @interface: the interface of the object
> + * @member: the name of the method in the interface
> + * @types: type signature for following method arguments
> + * @...: method arguments

> + * 'd' - 8-byte floating point, passed as a 'double'
> + * 's' - NULL terminated string, in UTF-8
> + * 'o' - NULL terminated string, representing a valid object path
> + * 'g' - NULL terminated string, representing a valid type signature

Technically, it's NUL-terminated (terminated by the one-byte character
NUL, not by the 4/8 byte pointer NULL), but we're inconsistent on this
so I don't care if you leave it.

> + * Example signatures, with their corresponding variadic
> + * args:

Thanks; this helps.

> + *
> + * - "a{sv}" - a hash table (aka array + dict entry)
> + *
> + *     (3, "email", "s", "joe blogs com", "age", "i", 35,
> + *      "address", "as", 3, "Some house", "Some road", "some city")

Wow, that adds up fast.

> +    ret = -1;
> +
> +    if (!(reply = dbus_connection_send_with_reply_and_block(conn,
> +                                                            call,
> +                                                            30 * 1000,

Worth a symbolic name for this magic number?

> +
> +/**
> + * virDBusMessageRead:
> + * @msg: the reply to decode
> + * @types: type signature for following return values
> + * @...: pointers in which to store return values
> + *
> + * The @types type signature is the same format as
> + * that used for the virDBusCallMethod. The difference
> + * is that each variadic parameter must be a pointer to
> + * be filled with the values. eg instead of passing an
> + * 'int', pass an 'int *'.

Are variants/structs/dicts allowed on decoding?  If so, an example of
how to interleave intermediate types alongside pointers for receiving
contents may make sense.

> +++ b/tests/virdbustest.c
> +#include "testutils.h"
> +
> +#define VERIFY(t, a, b, f)                                              \

These names are a bit cryptic, would it hurt to make them words?

> +
> +    if (!(msg = dbus_message_new_method_call("org.libvirt.test",
> +                                             "/org/libvirt/test",
> +                                             "org.libvirt.test.astrochicken",
> +                                             "cluck"))) {

bawk bawk b-gaawkk :)

> +
> +    if (virDBusMessageDecode(msg,
> +                             "svs",
> +                             &out_str1,
> +                             "i", &out_int32,
> +                             &out_str2) < 0) {

Ah, so you CAN pass a variant to decode.  I guess DBus is smart enough
to error out if any type signature fails to match the actual message
being decoded (that is, if I pass ["v", "i", &out_int32], but the
message was encoded with ["v", "s", "hello"], then I get a proper
message about type mismatch rather than some random integer which
happens to represent 32-bits of the pointer value of "hello")?  In fact,
it is worth a negative test, to prove that we handle mismatch in the
client's arguments vs. the message being decoded?

> +
> +    if (virDBusMessageDecode(msg,
> +                             "sais",
> +                             &out_str1,
> +                             3, &out_int32a, &out_int32b, &out_int32c,
> +                             &out_str2) < 0) {

Likewise, what happens if our array is too small or too large compared
to what we are decoding?  Too small is probably an error; does too large
work and just leave the extra pointers unmodified?

Overall, it looks pretty clean; I'm okay if you just post an interdiff
for the change you make to fix compilation instead of a full-blown v2.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


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