On 07/18/2013 07:27 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
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@xxxxxxxxx", "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
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list