[libvirt] [PATCH 0/6] Misc improvements and fixes to dbus APIs
This series is a bunch of fixes & improvements to the dbus APIs needed for the forthcoming new firewall APIs. Daniel P. Berrange (6): Refactor dbus helper methods for method calls Add DBus helper methods for creating reply messages Remove bogus unref in virDBusMessageRead Remove bogus call to dbus_set_error_from_message Introduce alternate way to encode/decode arrays in DBus messages Allow caller to handle DBus error messages src/libvirt_private.syms | 5 + src/util/virdbus.c | 426 ++++++++++++++++++++++++++++++++++++++++------- src/util/virdbus.h | 27 +++ src/util/virdbuspriv.h | 1 - src/util/virsystemd.c | 2 + tests/virdbustest.c | 93 +++++++++++ tests/virsystemdmock.c | 13 +- 7 files changed, 499 insertions(+), 68 deletions(-) -- 1.8.5.3
Split the virDBusMethodCall method into a couple of new methods virDBusCall, virDBusCreateMethod and virDBusCreateMethodV. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/libvirt_private.syms | 3 + src/util/virdbus.c | 207 +++++++++++++++++++++++++++++++++++++++-------- src/util/virdbus.h | 19 +++++ src/util/virdbuspriv.h | 1 - 4 files changed, 195 insertions(+), 35 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 3baf766..6091c67 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1155,8 +1155,11 @@ virCryptoHashString; # util/virdbus.h +virDBusCall; virDBusCallMethod; virDBusCloseSystemBus; +virDBusCreateMethod; +virDBusCreateMethodV; virDBusGetSessionBus; virDBusGetSystemBus; virDBusHasSystemBus; diff --git a/src/util/virdbus.c b/src/util/virdbus.c index e3236d8..d4e0381 100644 --- a/src/util/virdbus.c +++ b/src/util/virdbus.c @@ -1049,23 +1049,21 @@ int virDBusMessageDecode(DBusMessage* msg, # define VIR_DBUS_METHOD_CALL_TIMEOUT_MILLIS 30 * 1000 /** - * virDBusCallMethod: - * @conn: a DBus connection - * @replyout: pointer to receive reply message, or NULL + * virDBusCreateMethodV: + * @call: pointer to be filled with a method call message * @destination: bus identifier of the target service * @path: object path of the target service * @iface: the interface of the object * @member: the name of the method in the interface * @types: type signature for following method arguments - * @...: method arguments + * @args: method arguments * - * This invokes a method on a remote service on the - * DBus bus @conn. The @destination, @path, @iface + * This creates a DBus method call message and saves a + * pointer to it in @call. The @destination, @path, @iface * and @member parameters identify the object method to * be invoked. The optional @replyout parameter will be - * filled with any reply to the method call. The - * virDBusMethodReply method can be used to decode the - * return values. + * filled with any reply to the method call. The method + * can be later invoked using virDBusCall. * * The @types parameter is a DBus signature describing * the method call parameters which will be provided @@ -1166,38 +1164,91 @@ int virDBusMessageDecode(DBusMessage* msg, * (3, "email", "s", "joe@blogs.com", "age", "i", 35, * "address", "as", 3, "Some house", "Some road", "some city") */ - -int virDBusCallMethod(DBusConnection *conn, - DBusMessage **replyout, - const char *destination, - const char *path, - const char *iface, - const char *member, - const char *types, ...) +int virDBusCreateMethodV(DBusMessage **call, + const char *destination, + const char *path, + const char *iface, + const char *member, + const char *types, + va_list args) { - DBusMessage *call = NULL; - DBusMessage *reply = NULL; - DBusError error; int ret = -1; - va_list args; - dbus_error_init(&error); - - if (!(call = dbus_message_new_method_call(destination, - path, - iface, - member))) { + if (!(*call = dbus_message_new_method_call(destination, + path, + iface, + member))) { virReportOOMError(); goto cleanup; } + if (virDBusMessageEncodeArgs(*call, types, args) < 0) { + dbus_message_unref(*call); + *call = NULL; + goto cleanup; + } + + ret = 0; + cleanup: + return ret; +} + + +/** + * virDBusCreateMethod: + * @call: pointer to be filled with a method call message + * @destination: bus identifier of the target service + * @path: object path of the target service + * @iface: the interface of the object + * @member: the name of the method in the interface + * @types: type signature for following method arguments + * @...: method arguments + * + * See virDBusCreateMethodV for a description of the + * behaviour of this method. + */ +int virDBusCreateMethod(DBusMessage **call, + const char *destination, + const char *path, + const char *iface, + const char *member, + const char *types, ...) +{ + va_list args; + int ret; + va_start(args, types); - ret = virDBusMessageEncodeArgs(call, types, args); + ret = virDBusCreateMethodV(call, destination, path, + iface, member, types, args); va_end(args); - if (ret < 0) - goto cleanup; - ret = -1; + return ret; +} + + +/** + * virDBusCall: + * @conn: a DBus connection + * @call: pointer to a message to send + * @replyout: pointer to receive reply message, or NULL + * + * This invokes a method encoded in @call on a remote + * service on the DBus bus @conn. The optional @replyout + * parameter will be filled with any reply to the method + * call. The virDBusMethodReply method can be used to + * decode the return values. + * + * Returns 0 on success, or -1 upon error + */ +int virDBusCall(DBusConnection *conn, + DBusMessage *call, + DBusMessage **replyout) +{ + DBusMessage *reply = NULL; + DBusError error; + int ret = -1; + + dbus_error_init(&error); if (!(reply = dbus_connection_send_with_reply_and_block(conn, call, @@ -1217,10 +1268,8 @@ int virDBusCallMethod(DBusConnection *conn, ret = 0; -cleanup: + cleanup: dbus_error_free(&error); - if (call) - dbus_message_unref(call); if (reply) { if (ret == 0 && replyout) *replyout = reply; @@ -1232,6 +1281,62 @@ cleanup: /** + * virDBusCallMethod: + * @conn: a DBus connection + * @replyout: pointer to receive reply message, or NULL + * @destination: bus identifier of the target service + * @path: object path of the target service + * @iface: the interface of the object + * @member: the name of the method in the interface + * @types: type signature for following method arguments + * @...: method arguments + * + * This invokes a method on a remote service on the + * DBus bus @conn. The @destination, @path, @iface + * and @member parameters identify the object method to + * be invoked. The optional @replyout parameter will be + * filled with any reply to the method call. The + * virDBusMethodReply method can be used to decode the + * return values. + * + * The @types parameter is a DBus signature describing + * the method call parameters which will be provided + * as variadic args. See virDBusCreateMethodV for a + * description of this parameter. + * + * Returns: 0 on success, -1 on error + */ +int virDBusCallMethod(DBusConnection *conn, + DBusMessage **replyout, + const char *destination, + const char *path, + const char *iface, + const char *member, + const char *types, ...) +{ + DBusMessage *call = NULL; + int ret = -1; + va_list args; + + va_start(args, types); + ret = virDBusCreateMethodV(&call, destination, path, + iface, member, types, args); + va_end(args); + if (ret < 0) + goto cleanup; + + ret = -1; + + ret = virDBusCall(conn, call, replyout); + +cleanup: + if (call) + dbus_message_unref(call); + return ret; +} + + +/** * virDBusMessageRead: * @msg: the reply to decode * @types: type signature for following return values @@ -1369,6 +1474,40 @@ DBusConnection *virDBusGetSessionBus(void) return NULL; } +int virDBusCreateMethod(DBusMessage **call ATTRIBUTE_UNUSED, + const char *destination ATTRIBUTE_UNUSED, + const char *path ATTRIBUTE_UNUSED, + const char *iface ATTRIBUTE_UNUSED, + const char *member ATTRIBUTE_UNUSED, + const char *types ATTRIBUTE_UNUSED, ...) +{ + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("DBus support not compiled into this binary")); + return -1; +} + +int virDBusCreateMethodV(DBusMessage **call ATTRIBUTE_UNUSED, + const char *destination ATTRIBUTE_UNUSED, + const char *path ATTRIBUTE_UNUSED, + const char *iface ATTRIBUTE_UNUSED, + const char *member ATTRIBUTE_UNUSED, + const char *types ATTRIBUTE_UNUSED, + va_list args ATTRIBUTE_UNUSED) +{ + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("DBus support not compiled into this binary")); + return -1; +} + +int virDBusCall(DBusConnection *conn ATTRIBUTE_UNUSED, + DBusMessage *call ATTRIBUTE_UNUSED, + DBusMessage **reply ATTRIBUTE_UNUSED) +{ + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("DBus support not compiled into this binary")); + return -1; +} + int virDBusCallMethod(DBusConnection *conn ATTRIBUTE_UNUSED, DBusMessage **reply ATTRIBUTE_UNUSED, const char *destination ATTRIBUTE_UNUSED, diff --git a/src/util/virdbus.h b/src/util/virdbus.h index 1ca8641..e191a28 100644 --- a/src/util/virdbus.h +++ b/src/util/virdbus.h @@ -31,6 +31,8 @@ # endif # include "internal.h" +# include <stdarg.h> + void virDBusSetSharedBus(bool shared); DBusConnection *virDBusGetSystemBus(void); @@ -38,6 +40,20 @@ bool virDBusHasSystemBus(void); void virDBusCloseSystemBus(void); DBusConnection *virDBusGetSessionBus(void); +int virDBusCreateMethod(DBusMessage **call, + const char *destination, + const char *path, + const char *iface, + const char *member, + const char *types, ...); +int virDBusCreateMethodV(DBusMessage **call, + const char *destination, + const char *path, + const char *iface, + const char *member, + const char *types, + va_list args); + int virDBusCallMethod(DBusConnection *conn, DBusMessage **reply, const char *destination, @@ -45,6 +61,9 @@ int virDBusCallMethod(DBusConnection *conn, const char *iface, const char *member, const char *types, ...); +int virDBusCall(DBusConnection *conn, + DBusMessage *call, + DBusMessage **reply); int virDBusMessageRead(DBusMessage *msg, const char *types, ...); diff --git a/src/util/virdbuspriv.h b/src/util/virdbuspriv.h index a4ff655..d45fb25 100644 --- a/src/util/virdbuspriv.h +++ b/src/util/virdbuspriv.h @@ -24,7 +24,6 @@ # include "virdbus.h" -# include <stdarg.h> int virDBusMessageEncodeArgs(DBusMessage* msg, const char *types, -- 1.8.5.3
On 20.03.2014 13:28, Daniel P. Berrange wrote:
Split the virDBusMethodCall method into a couple of new methods virDBusCall, virDBusCreateMethod and virDBusCreateMethodV.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/libvirt_private.syms | 3 + src/util/virdbus.c | 207 +++++++++++++++++++++++++++++++++++++++-------- src/util/virdbus.h | 19 +++++ src/util/virdbuspriv.h | 1 - 4 files changed, 195 insertions(+), 35 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 3baf766..6091c67 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1155,8 +1155,11 @@ virCryptoHashString;
# util/virdbus.h +virDBusCall; virDBusCallMethod; virDBusCloseSystemBus; +virDBusCreateMethod; +virDBusCreateMethodV; virDBusGetSessionBus; virDBusGetSystemBus; virDBusHasSystemBus; diff --git a/src/util/virdbus.c b/src/util/virdbus.c index e3236d8..d4e0381 100644 --- a/src/util/virdbus.c +++ b/src/util/virdbus.c @@ -1049,23 +1049,21 @@ int virDBusMessageDecode(DBusMessage* msg, # define VIR_DBUS_METHOD_CALL_TIMEOUT_MILLIS 30 * 1000
/** - * virDBusCallMethod: - * @conn: a DBus connection - * @replyout: pointer to receive reply message, or NULL + * virDBusCreateMethodV: + * @call: pointer to be filled with a method call message * @destination: bus identifier of the target service * @path: object path of the target service * @iface: the interface of the object * @member: the name of the method in the interface * @types: type signature for following method arguments - * @...: method arguments + * @args: method arguments * - * This invokes a method on a remote service on the - * DBus bus @conn. The @destination, @path, @iface + * This creates a DBus method call message and saves a + * pointer to it in @call. The @destination, @path, @iface * and @member parameters identify the object method to * be invoked. The optional @replyout parameter will be - * filled with any reply to the method call. The - * virDBusMethodReply method can be used to decode the - * return values. + * filled with any reply to the method call. The method + * can be later invoked using virDBusCall. * * The @types parameter is a DBus signature describing * the method call parameters which will be provided @@ -1166,38 +1164,91 @@ int virDBusMessageDecode(DBusMessage* msg, * (3, "email", "s", "joe@blogs.com", "age", "i", 35, * "address", "as", 3, "Some house", "Some road", "some city") */ - -int virDBusCallMethod(DBusConnection *conn, - DBusMessage **replyout, - const char *destination, - const char *path, - const char *iface, - const char *member, - const char *types, ...) +int virDBusCreateMethodV(DBusMessage **call, + const char *destination, + const char *path, + const char *iface, + const char *member, + const char *types, + va_list args) { - DBusMessage *call = NULL; - DBusMessage *reply = NULL; - DBusError error; int ret = -1; - va_list args;
- dbus_error_init(&error); - - if (!(call = dbus_message_new_method_call(destination, - path, - iface, - member))) { + if (!(*call = dbus_message_new_method_call(destination, + path, + iface, + member))) { virReportOOMError(); goto cleanup; }
+ if (virDBusMessageEncodeArgs(*call, types, args) < 0) { + dbus_message_unref(*call); + *call = NULL; + goto cleanup; + } + + ret = 0; + cleanup:
Indentation's off
+ return ret; +} + + +/** + * virDBusCreateMethod: + * @call: pointer to be filled with a method call message + * @destination: bus identifier of the target service + * @path: object path of the target service + * @iface: the interface of the object + * @member: the name of the method in the interface + * @types: type signature for following method arguments + * @...: method arguments + * + * See virDBusCreateMethodV for a description of the + * behaviour of this method. + */ +int virDBusCreateMethod(DBusMessage **call, + const char *destination, + const char *path, + const char *iface, + const char *member, + const char *types, ...) +{ + va_list args; + int ret; + va_start(args, types); - ret = virDBusMessageEncodeArgs(call, types, args); + ret = virDBusCreateMethodV(call, destination, path, + iface, member, types, args); va_end(args); - if (ret < 0) - goto cleanup;
- ret = -1; + return ret; +} + + +/** + * virDBusCall: + * @conn: a DBus connection + * @call: pointer to a message to send + * @replyout: pointer to receive reply message, or NULL + * + * This invokes a method encoded in @call on a remote + * service on the DBus bus @conn. The optional @replyout + * parameter will be filled with any reply to the method + * call. The virDBusMethodReply method can be used to + * decode the return values. + * + * Returns 0 on success, or -1 upon error + */ +int virDBusCall(DBusConnection *conn, + DBusMessage *call, + DBusMessage **replyout) +{ + DBusMessage *reply = NULL; + DBusError error; + int ret = -1; + + dbus_error_init(&error);
if (!(reply = dbus_connection_send_with_reply_and_block(conn, call, @@ -1217,10 +1268,8 @@ int virDBusCallMethod(DBusConnection *conn,
ret = 0;
-cleanup: + cleanup:
And here as well.
dbus_error_free(&error); - if (call) - dbus_message_unref(call); if (reply) { if (ret == 0 && replyout) *replyout = reply;
ACK with that fixed. Michal
The test suites often have to create DBus method reply messages with payloads. Create two helpers for simplifying the process of creating replies with payloads. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/libvirt_private.syms | 2 ++ src/util/virdbus.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virdbus.h | 5 ++++ 3 files changed, 67 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 6091c67..6a89d1f 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1160,6 +1160,8 @@ virDBusCallMethod; virDBusCloseSystemBus; virDBusCreateMethod; virDBusCreateMethodV; +virDBusCreateReply; +virDBusCreateReplyV; virDBusGetSessionBus; virDBusGetSystemBus; virDBusHasSystemBus; diff --git a/src/util/virdbus.c b/src/util/virdbus.c index d4e0381..d5d3e01 100644 --- a/src/util/virdbus.c +++ b/src/util/virdbus.c @@ -1227,6 +1227,66 @@ int virDBusCreateMethod(DBusMessage **call, /** + * virDBusCreateReplyV: + * @reply: pointer to be filled with a method reply message + * @types: type signature for following method arguments + * @args: method arguments + * + * This creates a DBus method reply message and saves a + * pointer to it in @reply. + * + * The @types parameter is a DBus signature describing + * the method call parameters which will be provided + * as variadic args. See virDBusCreateMethodV for a + * description of this parameter. + */ +int virDBusCreateReplyV(DBusMessage **reply, + const char *types, + va_list args) +{ + int ret = -1; + + if (!(*reply = dbus_message_new(DBUS_MESSAGE_TYPE_METHOD_RETURN))) { + virReportOOMError(); + goto cleanup; + } + + if (virDBusMessageEncodeArgs(*reply, types, args) < 0) { + dbus_message_unref(*reply); + *reply = NULL; + goto cleanup; + } + + ret = 0; + cleanup: + return ret; +} + + +/** + * virDBusCreateReply: + * @reply: pointer to be filled with a method reply message + * @types: type signature for following method arguments + * @...: method arguments + * + * See virDBusCreateReplyV for a description of the + * behaviour of this method. + */ +int virDBusCreateReply(DBusMessage **reply, + const char *types, ...) +{ + va_list args; + int ret; + + va_start(args, types); + ret = virDBusCreateReplyV(reply, types, args); + va_end(args); + + return ret; +} + + +/** * virDBusCall: * @conn: a DBus connection * @call: pointer to a message to send diff --git a/src/util/virdbus.h b/src/util/virdbus.h index e191a28..4fbda87 100644 --- a/src/util/virdbus.h +++ b/src/util/virdbus.h @@ -53,6 +53,11 @@ int virDBusCreateMethodV(DBusMessage **call, const char *member, const char *types, va_list args); +int virDBusCreateReply(DBusMessage **reply, + const char *types, ...); +int virDBusCreateReplyV(DBusMessage **reply, + const char *types, + va_list args); int virDBusCallMethod(DBusConnection *conn, DBusMessage **reply, -- 1.8.5.3
On 20.03.2014 13:28, Daniel P. Berrange wrote:
The test suites often have to create DBus method reply messages with payloads. Create two helpers for simplifying the process of creating replies with payloads.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/libvirt_private.syms | 2 ++ src/util/virdbus.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virdbus.h | 5 ++++ 3 files changed, 67 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 6091c67..6a89d1f 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1160,6 +1160,8 @@ virDBusCallMethod; virDBusCloseSystemBus; virDBusCreateMethod; virDBusCreateMethodV; +virDBusCreateReply; +virDBusCreateReplyV; virDBusGetSessionBus; virDBusGetSystemBus; virDBusHasSystemBus; diff --git a/src/util/virdbus.c b/src/util/virdbus.c index d4e0381..d5d3e01 100644 --- a/src/util/virdbus.c +++ b/src/util/virdbus.c @@ -1227,6 +1227,66 @@ int virDBusCreateMethod(DBusMessage **call,
/** + * virDBusCreateReplyV: + * @reply: pointer to be filled with a method reply message + * @types: type signature for following method arguments + * @args: method arguments + * + * This creates a DBus method reply message and saves a + * pointer to it in @reply. + * + * The @types parameter is a DBus signature describing + * the method call parameters which will be provided + * as variadic args. See virDBusCreateMethodV for a + * description of this parameter. + */ +int virDBusCreateReplyV(DBusMessage **reply, + const char *types, + va_list args) +{ + int ret = -1; + + if (!(*reply = dbus_message_new(DBUS_MESSAGE_TYPE_METHOD_RETURN))) { + virReportOOMError(); + goto cleanup; + } + + if (virDBusMessageEncodeArgs(*reply, types, args) < 0) { + dbus_message_unref(*reply); + *reply = NULL; + goto cleanup; + } + + ret = 0; + cleanup:
Indentation's off.
+ return ret; +} + +
ACK with that fixed. Michal
On Thu, Mar 20, 2014 at 06:16:08PM +0100, Michal Privoznik wrote:
On 20.03.2014 13:28, Daniel P. Berrange wrote:
The test suites often have to create DBus method reply messages with payloads. Create two helpers for simplifying the process of creating replies with payloads.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/libvirt_private.syms | 2 ++ src/util/virdbus.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virdbus.h | 5 ++++ 3 files changed, 67 insertions(+)
+ ret = 0; + cleanup:
Indentation's off.
Do we actually have an indentation rule for labels ? 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 :|
On 03/20/2014 11:28 AM, Daniel P. Berrange wrote:
On Thu, Mar 20, 2014 at 06:16:08PM +0100, Michal Privoznik wrote:
On 20.03.2014 13:28, Daniel P. Berrange wrote:
The test suites often have to create DBus method reply messages with payloads. Create two helpers for simplifying the process of creating replies with payloads.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/libvirt_private.syms | 2 ++ src/util/virdbus.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virdbus.h | 5 ++++ 3 files changed, 67 insertions(+)
+ ret = 0; + cleanup:
Indentation's off.
Do we actually have an indentation rule for labels ?
Most code starts it in the first column, with no leading space. In some respects, emacs doesn't handle it well (it assumes anything in the first column is a function name, so it tries to treat the label as a function name when generating changelog templates); on the other hand, when I hit TAB on a label, emacs reindents it to the first column (that is, our .dir-locals.el requests c-file-style "K&R", and apparently that style includes putting labels at one indentation layer less than the rest of the code; so if you are labelling something that indents four spaces, the label gets indented 0 spaces). I've been going by the general rule of thumb that if emacs reindents something, then my style wasn't consistent with the bulk of the code; but I agree that HACKING doesn't actually mention this, and not everyone uses emacs. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Thu, Mar 20, 2014 at 11:39:18AM -0600, Eric Blake wrote:
On 03/20/2014 11:28 AM, Daniel P. Berrange wrote:
On Thu, Mar 20, 2014 at 06:16:08PM +0100, Michal Privoznik wrote:
On 20.03.2014 13:28, Daniel P. Berrange wrote:
The test suites often have to create DBus method reply messages with payloads. Create two helpers for simplifying the process of creating replies with payloads.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/libvirt_private.syms | 2 ++ src/util/virdbus.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virdbus.h | 5 ++++ 3 files changed, 67 insertions(+)
+ ret = 0; + cleanup:
Indentation's off.
Do we actually have an indentation rule for labels ?
Most code starts it in the first column, with no leading space.
In some respects, emacs doesn't handle it well (it assumes anything in the first column is a function name, so it tries to treat the label as a function name when generating changelog templates); on the other hand, when I hit TAB on a label, emacs reindents it to the first column (that is, our .dir-locals.el requests c-file-style "K&R", and apparently that style includes putting labels at one indentation layer less than the rest of the code; so if you are labelling something that indents four spaces, the label gets indented 0 spaces).
I've been going by the general rule of thumb that if emacs reindents something, then my style wasn't consistent with the bulk of the code; but I agree that HACKING doesn't actually mention this, and not everyone uses emacs.
Yep, I'm going with what emacs does, which is indent by 1 space here. 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 :|
On Fri, Mar 21, 2014 at 10:51:58 +0000, Daniel Berrange wrote:
On Thu, Mar 20, 2014 at 11:39:18AM -0600, Eric Blake wrote:
On 03/20/2014 11:28 AM, Daniel P. Berrange wrote:
On Thu, Mar 20, 2014 at 06:16:08PM +0100, Michal Privoznik wrote:
On 20.03.2014 13:28, Daniel P. Berrange wrote:
The test suites often have to create DBus method reply messages with payloads. Create two helpers for simplifying the process of creating replies with payloads.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/libvirt_private.syms | 2 ++ src/util/virdbus.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virdbus.h | 5 ++++ 3 files changed, 67 insertions(+)
+ ret = 0; + cleanup:
Indentation's off.
Do we actually have an indentation rule for labels ?
Most code starts it in the first column, with no leading space.
In some respects, emacs doesn't handle it well (it assumes anything in the first column is a function name, so it tries to treat the label as a function name when generating changelog templates); on the other hand, when I hit TAB on a label, emacs reindents it to the first column (that is, our .dir-locals.el requests c-file-style "K&R", and apparently that style includes putting labels at one indentation layer less than the rest of the code; so if you are labelling something that indents four spaces, the label gets indented 0 spaces).
I've been going by the general rule of thumb that if emacs reindents something, then my style wasn't consistent with the bulk of the code; but I agree that HACKING doesn't actually mention this, and not everyone uses emacs.
Yep, I'm going with what emacs does, which is indent by 1 space here.
And since it makes git diff produce better results too, I guess we could just start doing that everytime. Even vim can be told to indent labels by one space: :set cinoptions=L3 If you think 3 != 1, it's because 4 - 3 = 1, where 4 is the normal indent. Jirka
On 03/21/2014 04:51 AM, Daniel P. Berrange wrote:
I've been going by the general rule of thumb that if emacs reindents something, then my style wasn't consistent with the bulk of the code; but I agree that HACKING doesn't actually mention this, and not everyone uses emacs.
Yep, I'm going with what emacs does, which is indent by 1 space here.
Okay, then something is odd here, if your emacs indents by 1 space and mine doesn't. I'm not sure what is different between our two settings, but it would be nice to get it codified into .dir-locals.el so that it works the same for all emacs users. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
The virDBusMessageRead method should not have side-effects on the message parameter passed in, so unref'ing it is wrong. The caller should unref only when they decided they are done with it. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/util/virdbus.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/util/virdbus.c b/src/util/virdbus.c index d5d3e01..85f8e29 100644 --- a/src/util/virdbus.c +++ b/src/util/virdbus.c @@ -1419,7 +1419,6 @@ int virDBusMessageRead(DBusMessage *msg, ret = virDBusMessageDecodeArgs(msg, types, args); va_end(args); - dbus_message_unref(msg); return ret; } -- 1.8.5.3
On 20.03.2014 13:28, Daniel P. Berrange wrote:
The virDBusMessageRead method should not have side-effects on the message parameter passed in, so unref'ing it is wrong. The caller should unref only when they decided they are done with it.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/util/virdbus.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/src/util/virdbus.c b/src/util/virdbus.c index d5d3e01..85f8e29 100644 --- a/src/util/virdbus.c +++ b/src/util/virdbus.c @@ -1419,7 +1419,6 @@ int virDBusMessageRead(DBusMessage *msg, ret = virDBusMessageDecodeArgs(msg, types, args); va_end(args);
- dbus_message_unref(msg); return ret; }
ACK, and there's no new mem leak as this functions is not used anywhere yet. Michal
The dbus_connection_send_with_reply_and_block method will automatically call dbus_set_error_from_message for us. We mistakenly thought we had todo it because of a flaw in the systemd unit test mock impl. The latter should have directly set the error object, instead of creating an error message object. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/util/virdbus.c | 7 ------- tests/virsystemdmock.c | 13 +++---------- 2 files changed, 3 insertions(+), 17 deletions(-) diff --git a/src/util/virdbus.c b/src/util/virdbus.c index 85f8e29..8a978e5 100644 --- a/src/util/virdbus.c +++ b/src/util/virdbus.c @@ -1319,13 +1319,6 @@ int virDBusCall(DBusConnection *conn, goto cleanup; } - if (dbus_set_error_from_message(&error, - reply)) { - virReportDBusServiceError(error.message ? error.message : "unknown error", - error.name); - goto cleanup; - } - ret = 0; cleanup: diff --git a/tests/virsystemdmock.c b/tests/virsystemdmock.c index b4fcf6e..23167db 100644 --- a/tests/virsystemdmock.c +++ b/tests/virsystemdmock.c @@ -70,16 +70,9 @@ DBusMessage *dbus_connection_send_with_reply_and_block(DBusConnection *connectio if (STREQ(service, "org.freedesktop.machine1")) { if (getenv("FAIL_BAD_SERVICE")) { - DBusMessageIter iter; - const char *error_message = "Something went wrong creating the machine"; - if (!(reply = dbus_message_new(DBUS_MESSAGE_TYPE_ERROR))) - return NULL; - dbus_message_set_error_name(reply, "org.freedesktop.systemd.badthing"); - dbus_message_iter_init_append(reply, &iter); - if (!dbus_message_iter_append_basic(&iter, - DBUS_TYPE_STRING, - &error_message)) - goto error; + dbus_set_error_const(error, + "org.freedesktop.systemd.badthing", + "Something went wrong creating the machine"); } else { reply = dbus_message_new(DBUS_MESSAGE_TYPE_METHOD_RETURN); } -- 1.8.5.3
On 20.03.2014 13:28, Daniel P. Berrange wrote:
The dbus_connection_send_with_reply_and_block method will automatically call dbus_set_error_from_message for us. We mistakenly thought we had todo it because of a flaw in the systemd unit test mock impl. The latter should have directly set the error object, instead of creating an error message object.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/util/virdbus.c | 7 ------- tests/virsystemdmock.c | 13 +++---------- 2 files changed, 3 insertions(+), 17 deletions(-)
diff --git a/src/util/virdbus.c b/src/util/virdbus.c index 85f8e29..8a978e5 100644 --- a/src/util/virdbus.c +++ b/src/util/virdbus.c @@ -1319,13 +1319,6 @@ int virDBusCall(DBusConnection *conn, goto cleanup; }
- if (dbus_set_error_from_message(&error, - reply)) { - virReportDBusServiceError(error.message ? error.message : "unknown error", - error.name); - goto cleanup; - } - ret = 0;
cleanup: diff --git a/tests/virsystemdmock.c b/tests/virsystemdmock.c index b4fcf6e..23167db 100644 --- a/tests/virsystemdmock.c +++ b/tests/virsystemdmock.c @@ -70,16 +70,9 @@ DBusMessage *dbus_connection_send_with_reply_and_block(DBusConnection *connectio
if (STREQ(service, "org.freedesktop.machine1")) { if (getenv("FAIL_BAD_SERVICE")) { - DBusMessageIter iter; - const char *error_message = "Something went wrong creating the machine"; - if (!(reply = dbus_message_new(DBUS_MESSAGE_TYPE_ERROR))) - return NULL; - dbus_message_set_error_name(reply, "org.freedesktop.systemd.badthing"); - dbus_message_iter_init_append(reply, &iter); - if (!dbus_message_iter_append_basic(&iter, - DBUS_TYPE_STRING, - &error_message)) - goto error; + dbus_set_error_const(error, + "org.freedesktop.systemd.badthing", + "Something went wrong creating the machine"); } else { reply = dbus_message_new(DBUS_MESSAGE_TYPE_METHOD_RETURN); }
ACK Michal
Currently the DBus helper APIs require the values for an array to be passed inline in the variadic argument list. This change introduces support for passing arrays using a pointer to a plain C array of the basic type. This is of particular benefit for decoding messages when you don't know how many array elements are being received. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/util/virdbus.c | 121 ++++++++++++++++++++++++++++++++++++++++++++++------ tests/virdbustest.c | 93 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 201 insertions(+), 13 deletions(-) diff --git a/src/util/virdbus.c b/src/util/virdbus.c index 8a978e5..d208646 100644 --- a/src/util/virdbus.c +++ b/src/util/virdbus.c @@ -456,7 +456,7 @@ static int virDBusTypeStackPush(virDBusTypeStack **stack, (*stack)[(*nstack) - 1].types = types; (*stack)[(*nstack) - 1].nstruct = nstruct; (*stack)[(*nstack) - 1].narray = narray; - VIR_DEBUG("Pushed '%s'", types); + VIR_DEBUG("Pushed types='%s' nstruct=%zu narray=%zu", types, nstruct, narray); return 0; } @@ -478,7 +478,7 @@ static int virDBusTypeStackPop(virDBusTypeStack **stack, *types = (*stack)[(*nstack) - 1].types; *nstruct = (*stack)[(*nstack) - 1].nstruct; *narray = (*stack)[(*nstack) - 1].narray; - VIR_DEBUG("Popped '%s'", *types); + VIR_DEBUG("Popped types='%s' nstruct=%zu narray=%zu", *types, *nstruct, *narray); VIR_SHRINK_N(*stack, *nstack, 1); return 0; @@ -501,16 +501,25 @@ static void virDBusTypeStackFree(virDBusTypeStack **stack, # define SET_NEXT_VAL(dbustype, vargtype, sigtype, fmt) \ do { \ - dbustype x = (dbustype)va_arg(args, vargtype); \ + dbustype x; \ + if (arrayref) { \ + vargtype *valarray = arrayptr; \ + x = (dbustype)*valarray; \ + valarray++; \ + arrayptr = valarray; \ + } else { \ + x = (dbustype)va_arg(args, vargtype); \ + } \ if (!dbus_message_iter_append_basic(iter, sigtype, &x)) { \ virReportError(VIR_ERR_INTERNAL_ERROR, \ - _("Cannot append basic type %s"), #vargtype); \ + _("Cannot append basic type %s"), #vargtype);\ goto cleanup; \ } \ - VIR_DEBUG("Appended basic type '" #dbustype "' varg '" #vargtype \ + VIR_DEBUG("Appended basic type '" #dbustype "' varg '" #vargtype\ "' sig '%c' val '" fmt "'", sigtype, (vargtype)x); \ } while (0) + static int virDBusMessageIterEncode(DBusMessageIter *rootiter, const char *types, @@ -519,6 +528,8 @@ virDBusMessageIterEncode(DBusMessageIter *rootiter, int ret = -1; size_t narray; size_t nstruct; + bool arrayref = false; + void *arrayptr = NULL; virDBusTypeStack *stack = NULL; size_t nstack = 0; size_t siglen; @@ -544,6 +555,8 @@ virDBusMessageIterEncode(DBusMessageIter *rootiter, (narray == (size_t)-1 && nstruct == 0)) { DBusMessageIter *thisiter = iter; + arrayref = false; + arrayptr = NULL; VIR_DEBUG("Popping iter=%p", iter); if (nstack == 0) break; @@ -616,12 +629,32 @@ virDBusMessageIterEncode(DBusMessageIter *rootiter, break; case DBUS_TYPE_ARRAY: + arrayptr = NULL; + if (t[1] == '&') { + VIR_DEBUG("Got array ref"); + t++; + types++; + nstruct--; + arrayref = true; + } else { + VIR_DEBUG("Got array non-ref"); + arrayref = false; + } + if (virDBusSignatureLength(t + 1, &siglen) < 0) goto cleanup; if (VIR_STRNDUP(contsig, t + 1, siglen) < 0) goto cleanup; + if (arrayref && (strlen(contsig) > 1 || + !virDBusIsBasicType(*contsig))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Got array ref but '%s' is not a single basic type"), + contsig); + goto cleanup; + } + if (narray == (size_t)-1) { types += siglen; nstruct -= siglen; @@ -644,7 +677,9 @@ virDBusMessageIterEncode(DBusMessageIter *rootiter, newiter = NULL; types = t + 1; nstruct = siglen; - narray = va_arg(args, int); + narray = (size_t)va_arg(args, int); + if (arrayref) + arrayptr = va_arg(args, void *); break; case DBUS_TYPE_VARIANT: @@ -710,8 +745,9 @@ virDBusMessageIterEncode(DBusMessageIter *rootiter, default: virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unknown type in signature '%s'"), - types); + _("Unknown type '%c' in signature '%s'"), + *t, types); + goto cleanup; } } @@ -739,7 +775,16 @@ cleanup: # define GET_NEXT_VAL(dbustype, vargtype, fmt) \ do { \ - dbustype *x = (dbustype *)va_arg(args, vargtype *); \ + dbustype *x; \ + if (arrayref) { \ + vargtype **xptrptr = arrayptr; \ + if (VIR_EXPAND_N(*xptrptr, *narrayptr, 1) < 0) \ + goto cleanup; \ + x = (dbustype *)(*xptrptr + (*narrayptr - 1)); \ + VIR_DEBUG("Expanded to %zu", *narrayptr); \ + } else { \ + x = (dbustype *)va_arg(args, vargtype *); \ + } \ dbus_message_iter_get_basic(iter, x); \ VIR_DEBUG("Read basic type '" #dbustype "' varg '" #vargtype \ "' val '" fmt "'", (vargtype)*x); \ @@ -754,6 +799,9 @@ virDBusMessageIterDecode(DBusMessageIter *rootiter, int ret = -1; size_t narray; size_t nstruct; + bool arrayref = false; + void *arrayptr = NULL; + size_t *narrayptr = 0; virDBusTypeStack *stack = NULL; size_t nstack = 0; size_t siglen; @@ -780,6 +828,8 @@ virDBusMessageIterDecode(DBusMessageIter *rootiter, (narray == (size_t)-1 && nstruct == 0)) { DBusMessageIter *thisiter = iter; + arrayref = false; + arrayptr = NULL; VIR_DEBUG("Popping iter=%p", iter); if (nstack == 0) break; @@ -849,7 +899,16 @@ virDBusMessageIterDecode(DBusMessageIter *rootiter, case DBUS_TYPE_OBJECT_PATH: case DBUS_TYPE_SIGNATURE: do { - char **x = (char **)va_arg(args, char **); + char **x; + if (arrayref) { + char ***xptrptr = arrayptr; + if (VIR_EXPAND_N(*xptrptr, *narrayptr, 1) < 0) + goto cleanup; + x = (char **)(*xptrptr + (*narrayptr - 1)); + VIR_DEBUG("Expanded to %zu", *narrayptr); + } else { + x = (char **)va_arg(args, char **); + } char *s; dbus_message_iter_get_basic(iter, &s); if (VIR_STRDUP(*x, s) < 0) @@ -860,6 +919,18 @@ virDBusMessageIterDecode(DBusMessageIter *rootiter, break; case DBUS_TYPE_ARRAY: + arrayptr = NULL; + if (t[1] == '&') { + VIR_DEBUG("Got array ref"); + t++; + types++; + nstruct--; + arrayref = true; + } else { + VIR_DEBUG("Got array non-ref"); + arrayref = false; + } + advanceiter = false; if (virDBusSignatureLength(t + 1, &siglen) < 0) goto cleanup; @@ -867,6 +938,14 @@ virDBusMessageIterDecode(DBusMessageIter *rootiter, if (VIR_STRNDUP(contsig, t + 1, siglen) < 0) goto cleanup; + if (arrayref && (strlen(contsig) > 1 || + !virDBusIsBasicType(*contsig))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Got array ref but '%s' is not a single basic type"), + contsig); + goto cleanup; + } + if (narray == (size_t)-1) { types += siglen; nstruct -= siglen; @@ -885,7 +964,14 @@ virDBusMessageIterDecode(DBusMessageIter *rootiter, newiter = NULL; types = t + 1; nstruct = siglen; - narray = va_arg(args, int); + if (arrayref) { + narrayptr = va_arg(args, size_t *); + arrayptr = va_arg(args, void *); + *narrayptr = 0; + *(char **)arrayptr = NULL; + } else { + narray = va_arg(args, int); + } break; case DBUS_TYPE_VARIANT: @@ -945,8 +1031,17 @@ virDBusMessageIterDecode(DBusMessageIter *rootiter, default: virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unknown type in signature '%s'"), - types); + _("Unknown type '%c' in signature '%s'"), + *t, types); + goto cleanup; + } + + if (arrayref) { + if (*t == '&' || + dbus_message_iter_has_next(iter)) + narray = 1; + else + narray = 0; } VIR_DEBUG("After stack=%zu array=%zu struct=%zu type='%s'", diff --git a/tests/virdbustest.c b/tests/virdbustest.c index 9a6c4c6..2269c8d 100644 --- a/tests/virdbustest.c +++ b/tests/virdbustest.c @@ -228,6 +228,97 @@ cleanup: return ret; } +static int testMessageArrayRef(const void *args ATTRIBUTE_UNUSED) +{ + DBusMessage *msg = NULL; + int ret = -1; + const char *in_str1 = "Hello"; + int in_int32[] = { + 100000000, 2000000000, -2000000000 + }; + const char *in_strv1[] = { + "Fishfood", + }; + const char *in_strv2[] = { + "Hello", "World", + }; + int *out_int32 = NULL; + size_t out_nint32 = 0; + char **out_strv1 = NULL; + char **out_strv2 = NULL; + size_t out_nstrv1 = 0; + size_t out_nstrv2 = 0; + const char *in_str2 = "World"; + char *out_str1 = NULL, *out_str2 = NULL; + + if (!(msg = dbus_message_new_method_call("org.libvirt.test", + "/org/libvirt/test", + "org.libvirt.test.astrochicken", + "cluck"))) { + VIR_DEBUG("Failed to allocate method call"); + goto cleanup; + } + + if (virDBusMessageEncode(msg, + "sa&sa&ia&ss", + in_str1, + 1, in_strv1, + 3, in_int32, + 2, in_strv2, + in_str2) < 0) { + VIR_DEBUG("Failed to encode arguments"); + goto cleanup; + } + + if (virDBusMessageDecode(msg, + "sa&sa&ia&ss", + &out_str1, + &out_nstrv1, &out_strv1, + &out_nint32, &out_int32, + &out_nstrv2, &out_strv2, + &out_str2) < 0) { + VIR_DEBUG("Failed to decode arguments"); + goto cleanup; + } + + + VERIFY_STR("str1", in_str1, out_str1, "%s"); + if (out_nstrv1 != 1) { + fprintf(stderr, "Expected 1 string, but got %zu\n", + out_nstrv1); + goto cleanup; + } + VERIFY_STR("strv1[0]", in_strv1[0], out_strv1[0], "%s"); + + if (out_nint32 != 3) { + fprintf(stderr, "Expected 3 integers, but got %zu\n", + out_nint32); + goto cleanup; + } + VERIFY("int32a", in_int32[0], out_int32[0], "%d"); + VERIFY("int32b", in_int32[1], out_int32[1], "%d"); + VERIFY("int32c", in_int32[2], out_int32[2], "%d"); + + if (out_nstrv2 != 2) { + fprintf(stderr, "Expected 2 strings, but got %zu\n", + out_nstrv2); + goto cleanup; + } + VERIFY_STR("strv2[0]", in_strv2[0], out_strv2[0], "%s"); + VERIFY_STR("strv2[1]", in_strv2[1], out_strv2[1], "%s"); + + VERIFY_STR("str2", in_str2, out_str2, "%s"); + + ret = 0; + +cleanup: + VIR_FREE(out_int32); + VIR_FREE(out_str1); + VIR_FREE(out_str2); + dbus_message_unref(msg); + return ret; +} + static int testMessageStruct(const void *args ATTRIBUTE_UNUSED) { DBusMessage *msg = NULL; @@ -385,6 +476,8 @@ mymain(void) ret = -1; if (virtTestRun("Test message array ", testMessageArray, NULL) < 0) ret = -1; + if (virtTestRun("Test message array ref ", testMessageArrayRef, NULL) < 0) + ret = -1; if (virtTestRun("Test message struct ", testMessageStruct, NULL) < 0) ret = -1; if (virtTestRun("Test message dict ", testMessageDict, NULL) < 0) -- 1.8.5.3
On 20.03.2014 13:28, Daniel P. Berrange wrote:
Currently the DBus helper APIs require the values for an array to be passed inline in the variadic argument list. This change introduces support for passing arrays using a pointer to a plain C array of the basic type. This is of particular benefit for decoding messages when you don't know how many array elements are being received.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/util/virdbus.c | 121 ++++++++++++++++++++++++++++++++++++++++++++++------ tests/virdbustest.c | 93 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 201 insertions(+), 13 deletions(-)
Nice, we have a test too! ACK Michal
On Thu, Mar 20, 2014 at 06:15:48PM +0100, Michal Privoznik wrote:
On 20.03.2014 13:28, Daniel P. Berrange wrote:
Currently the DBus helper APIs require the values for an array to be passed inline in the variadic argument list. This change introduces support for passing arrays using a pointer to a plain C array of the basic type. This is of particular benefit for decoding messages when you don't know how many array elements are being received.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/util/virdbus.c | 121 ++++++++++++++++++++++++++++++++++++++++++++++------ tests/virdbustest.c | 93 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 201 insertions(+), 13 deletions(-)
Nice, we have a test too!
Mostly because I learnt the hard way that it is damn near impossible to get this DBus code right without a test suite for it :-) 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 :|
The caller may not want all DBus error conditions to be turned into libvirt errors, so provide a way for the caller to get back the full DBusError object. They can then check the errors and only report those that they consider to be fatal. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/util/virdbus.c | 46 ++++++++++++++++++++++++++++++++++++---------- src/util/virdbus.h | 5 ++++- src/util/virsystemd.c | 2 ++ 3 files changed, 42 insertions(+), 11 deletions(-) diff --git a/src/util/virdbus.c b/src/util/virdbus.c index d208646..1eaac23 100644 --- a/src/util/virdbus.c +++ b/src/util/virdbus.c @@ -1386,6 +1386,7 @@ int virDBusCreateReply(DBusMessage **reply, * @conn: a DBus connection * @call: pointer to a message to send * @replyout: pointer to receive reply message, or NULL + * @error: pointer to receive error message * * This invokes a method encoded in @call on a remote * service on the DBus bus @conn. The optional @replyout @@ -1393,31 +1394,43 @@ int virDBusCreateReply(DBusMessage **reply, * call. The virDBusMethodReply method can be used to * decode the return values. * + * If @error is NULL then a libvirt error will be raised + * when a DBus error is received and the return value will + * be -1. If @error is non-NULL then any DBus error will + * be saved into that object and the return value will + * be 0. + * * Returns 0 on success, or -1 upon error */ int virDBusCall(DBusConnection *conn, DBusMessage *call, - DBusMessage **replyout) + DBusMessage **replyout, + DBusError *error) { DBusMessage *reply = NULL; - DBusError error; + DBusError localerror; int ret = -1; - dbus_error_init(&error); + if (!error) + dbus_error_init(&localerror); if (!(reply = dbus_connection_send_with_reply_and_block(conn, call, VIR_DBUS_METHOD_CALL_TIMEOUT_MILLIS, - &error))) { - virReportDBusServiceError(error.message ? error.message : "unknown error", - error.name); + error ? error : &localerror))) { + if (error) + ret = 0; + else + virReportDBusServiceError(localerror.message ? localerror.message : "unknown error", + localerror.name); goto cleanup; } ret = 0; cleanup: - dbus_error_free(&error); + if (!error) + dbus_error_free(&localerror); if (reply) { if (ret == 0 && replyout) *replyout = reply; @@ -1452,10 +1465,20 @@ int virDBusCall(DBusConnection *conn, * as variadic args. See virDBusCreateMethodV for a * description of this parameter. * - * Returns: 0 on success, -1 on error + * + * If @error is NULL then a libvirt error will be raised + * when a DBus error is received and the return value will + * be -1. If @error is non-NULL then any DBus error will + * be saved into that object and the return value will + * be 0. If an error occurs while encoding method args + * the return value will always be -1 regardless of whether + * @error is set. + * + * Returns 0 on success, or -1 upon error */ int virDBusCallMethod(DBusConnection *conn, DBusMessage **replyout, + DBusError *error, const char *destination, const char *path, const char *iface, @@ -1475,7 +1498,7 @@ int virDBusCallMethod(DBusConnection *conn, ret = -1; - ret = virDBusCall(conn, call, replyout); + ret = virDBusCall(conn, call, replyout, error); cleanup: if (call) @@ -1525,6 +1548,7 @@ static int virDBusIsServiceInList(const char *listMethod, const char *name) if (virDBusCallMethod(conn, &reply, + NULL, "org.freedesktop.DBus", "/org/freedesktop/DBus", "org.freedesktop.DBus", @@ -1648,7 +1672,8 @@ int virDBusCreateMethodV(DBusMessage **call ATTRIBUTE_UNUSED, int virDBusCall(DBusConnection *conn ATTRIBUTE_UNUSED, DBusMessage *call ATTRIBUTE_UNUSED, - DBusMessage **reply ATTRIBUTE_UNUSED) + DBusMessage **reply ATTRIBUTE_UNUSED, + DBusError *error ATTRIBUTE_UNUSED) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("DBus support not compiled into this binary")); @@ -1657,6 +1682,7 @@ int virDBusCall(DBusConnection *conn ATTRIBUTE_UNUSED, int virDBusCallMethod(DBusConnection *conn ATTRIBUTE_UNUSED, DBusMessage **reply ATTRIBUTE_UNUSED, + DBusError *error ATTRIBUTE_UNUSED, const char *destination ATTRIBUTE_UNUSED, const char *path ATTRIBUTE_UNUSED, const char *iface ATTRIBUTE_UNUSED, diff --git a/src/util/virdbus.h b/src/util/virdbus.h index 4fbda87..0f21821 100644 --- a/src/util/virdbus.h +++ b/src/util/virdbus.h @@ -28,6 +28,7 @@ # else # define DBusConnection void # define DBusMessage void +# define DBusError void # endif # include "internal.h" @@ -61,6 +62,7 @@ int virDBusCreateReplyV(DBusMessage **reply, int virDBusCallMethod(DBusConnection *conn, DBusMessage **reply, + DBusError *error, const char *destination, const char *path, const char *iface, @@ -68,7 +70,8 @@ int virDBusCallMethod(DBusConnection *conn, const char *types, ...); int virDBusCall(DBusConnection *conn, DBusMessage *call, - DBusMessage **reply); + DBusMessage **reply, + DBusError *error); int virDBusMessageRead(DBusMessage *msg, const char *types, ...); diff --git a/src/util/virsystemd.c b/src/util/virsystemd.c index f2eeb8c..0281158 100644 --- a/src/util/virsystemd.c +++ b/src/util/virsystemd.c @@ -236,6 +236,7 @@ int virSystemdCreateMachine(const char *name, VIR_DEBUG("Attempting to create machine via systemd"); if (virDBusCallMethod(conn, NULL, + NULL, "org.freedesktop.machine1", "/org/freedesktop/machine1", "org.freedesktop.machine1.Manager", @@ -301,6 +302,7 @@ int virSystemdTerminateMachine(const char *name, VIR_DEBUG("Attempting to terminate machine via systemd"); if (virDBusCallMethod(conn, NULL, + NULL, "org.freedesktop.machine1", "/org/freedesktop/machine1", "org.freedesktop.machine1.Manager", -- 1.8.5.3
On 20.03.2014 13:28, Daniel P. Berrange wrote:
The caller may not want all DBus error conditions to be turned into libvirt errors, so provide a way for the caller to get back the full DBusError object. They can then check the errors and only report those that they consider to be fatal.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/util/virdbus.c | 46 ++++++++++++++++++++++++++++++++++++---------- src/util/virdbus.h | 5 ++++- src/util/virsystemd.c | 2 ++ 3 files changed, 42 insertions(+), 11 deletions(-)
ACK Michal
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Jiri Denemark -
Michal Privoznik