sd-bus: always fill in sd_bus_error paramters, on error

Whenever one of our calls is invoked with a non-NULL, writable
sd_bus_error parameter, let's fill in some valid error on failure. We
previously only filled in remote errors, but never local errors, which is
hard to handle by users. Hence, let's clean this up to always fill in
the error.

This introduces a new bus_assert_return() macro that works like
assert_return() but optionally also initializes a bus_error struct.

Fixes #224.

Based on a patch by Umut Tezduyar.

diff --git a/src/libsystemd/sd-bus/bus-convenience.c b/src/libsystemd/sd-bus/bus-convenience.c
index 87898449e6c40792f20421164751b260ad62bbe5..79747e058b0552b82f7d26a1cfef9a35a8308829 100644 (file)
--- a/src/libsystemd/sd-bus/bus-convenience.c
+++ b/src/libsystemd/sd-bus/bus-convenience.c
@@ -107,15 +107,17 @@ _public_ int sd_bus_call_method(
         _cleanup_bus_message_unref_ sd_bus_message *m = NULL;
         int r;
 
-        assert_return(bus, -EINVAL);
-        assert_return(!bus_pid_changed(bus), -ECHILD);
+        bus_assert_return(bus, -EINVAL, error);
+        bus_assert_return(!bus_pid_changed(bus), -ECHILD, error);
 
-        if (!BUS_IS_OPEN(bus->state))
-                return -ENOTCONN;
+        if (!BUS_IS_OPEN(bus->state)) {
+                r = -ENOTCONN;
+                goto fail;
+        }
 
         r = sd_bus_message_new_method_call(bus, &m, destination, path, interface, member);
         if (r < 0)
-                return r;
+                goto fail;
 
         if (!isempty(types)) {
                 va_list ap;
@@ -124,10 +126,13 @@ _public_ int sd_bus_call_method(
                 r = bus_message_append_ap(m, types, ap);
                 va_end(ap);
                 if (r < 0)
-                        return r;
+                        goto fail;
         }
 
         return sd_bus_call(bus, m, 0, error, reply);
+
+fail:
+        return sd_bus_error_set_errno(error, r);
 }
 
 _public_ int sd_bus_reply_method_return(
@@ -289,15 +294,17 @@ _public_ int sd_bus_get_property(
         sd_bus_message *rep = NULL;
         int r;
 
-        assert_return(bus, -EINVAL);
-        assert_return(isempty(interface) || interface_name_is_valid(interface), -EINVAL);
-        assert_return(member_name_is_valid(member), -EINVAL);
-        assert_return(reply, -EINVAL);
-        assert_return(signature_is_single(type, false), -EINVAL);
-        assert_return(!bus_pid_changed(bus), -ECHILD);
+        bus_assert_return(bus, -EINVAL, error);
+        bus_assert_return(isempty(interface) || interface_name_is_valid(interface), -EINVAL, error);
+        bus_assert_return(member_name_is_valid(member), -EINVAL, error);
+        bus_assert_return(reply, -EINVAL, error);
+        bus_assert_return(signature_is_single(type, false), -EINVAL, error);
+        bus_assert_return(!bus_pid_changed(bus), -ECHILD, error);
 
-        if (!BUS_IS_OPEN(bus->state))
-                return -ENOTCONN;
+        if (!BUS_IS_OPEN(bus->state)) {
+                r = -ENOTCONN;
+                goto fail;
+        }
 
         r = sd_bus_call_method(bus, destination, path, "org.freedesktop.DBus.Properties", "Get", error, &rep, "ss", strempty(interface), member);
         if (r < 0)
@@ -306,11 +313,14 @@ _public_ int sd_bus_get_property(
         r = sd_bus_message_enter_container(rep, 'v', type);
         if (r < 0) {
                 sd_bus_message_unref(rep);
-                return r;
+                goto fail;
         }
 
         *reply = rep;
         return 0;
+
+fail:
+        return sd_bus_error_set_errno(error, r);
 }
 
 _public_ int sd_bus_get_property_trivial(
@@ -325,15 +335,17 @@ _public_ int sd_bus_get_property_trivial(
         _cleanup_bus_message_unref_ sd_bus_message *reply = NULL;
         int r;
 
-        assert_return(bus, -EINVAL);
-        assert_return(isempty(interface) || interface_name_is_valid(interface), -EINVAL);
-        assert_return(member_name_is_valid(member), -EINVAL);
-        assert_return(bus_type_is_trivial(type), -EINVAL);
-        assert_return(ptr, -EINVAL);
-        assert_return(!bus_pid_changed(bus), -ECHILD);
+        bus_assert_return(bus, -EINVAL, error);
+        bus_assert_return(isempty(interface) || interface_name_is_valid(interface), -EINVAL, error);
+        bus_assert_return(member_name_is_valid(member), -EINVAL, error);
+        bus_assert_return(bus_type_is_trivial(type), -EINVAL, error);
+        bus_assert_return(ptr, -EINVAL, error);
+        bus_assert_return(!bus_pid_changed(bus), -ECHILD, error);
 
-        if (!BUS_IS_OPEN(bus->state))
-                return -ENOTCONN;
+        if (!BUS_IS_OPEN(bus->state)) {
+                r = -ENOTCONN;
+                goto fail;
+        }
 
         r = sd_bus_call_method(bus, destination, path, "org.freedesktop.DBus.Properties", "Get", error, &reply, "ss", strempty(interface), member);
         if (r < 0)
@@ -341,13 +353,16 @@ _public_ int sd_bus_get_property_trivial(
 
         r = sd_bus_message_enter_container(reply, 'v', CHAR_TO_STR(type));
         if (r < 0)
-                return r;
+                goto fail;
 
         r = sd_bus_message_read_basic(reply, type, ptr);
         if (r < 0)
-                return r;
+                goto fail;
 
         return 0;
+
+fail:
+        return sd_bus_error_set_errno(error, r);
 }
 
 _public_ int sd_bus_get_property_string(
@@ -364,14 +379,16 @@ _public_ int sd_bus_get_property_string(
         char *n;
         int r;
 
-        assert_return(bus, -EINVAL);
-        assert_return(isempty(interface) || interface_name_is_valid(interface), -EINVAL);
-        assert_return(member_name_is_valid(member), -EINVAL);
-        assert_return(ret, -EINVAL);
-        assert_return(!bus_pid_changed(bus), -ECHILD);
+        bus_assert_return(bus, -EINVAL, error);
+        bus_assert_return(isempty(interface) || interface_name_is_valid(interface), -EINVAL, error);
+        bus_assert_return(member_name_is_valid(member), -EINVAL, error);
+        bus_assert_return(ret, -EINVAL, error);
+        bus_assert_return(!bus_pid_changed(bus), -ECHILD, error);
 
-        if (!BUS_IS_OPEN(bus->state))
-                return -ENOTCONN;
+        if (!BUS_IS_OPEN(bus->state)) {
+                r = -ENOTCONN;
+                goto fail;
+        }
 
         r = sd_bus_call_method(bus, destination, path, "org.freedesktop.DBus.Properties", "Get", error, &reply, "ss", strempty(interface), member);
         if (r < 0)
@@ -379,18 +396,23 @@ _public_ int sd_bus_get_property_string(
 
         r = sd_bus_message_enter_container(reply, 'v', "s");
         if (r < 0)
-                return r;
+                goto fail;
 
         r = sd_bus_message_read_basic(reply, 's', &s);
         if (r < 0)
-                return r;
+                goto fail;
 
         n = strdup(s);
-        if (!n)
-                return -ENOMEM;
+        if (!n) {
+                r = -ENOMEM;
+                goto fail;
+        }
 
         *ret = n;
         return 0;
+
+fail:
+        return sd_bus_error_set_errno(error, r);
 }
 
 _public_ int sd_bus_get_property_strv(
@@ -405,14 +427,16 @@ _public_ int sd_bus_get_property_strv(
         _cleanup_bus_message_unref_ sd_bus_message *reply = NULL;
         int r;
 
-        assert_return(bus, -EINVAL);
-        assert_return(isempty(interface) || interface_name_is_valid(interface), -EINVAL);
-        assert_return(member_name_is_valid(member), -EINVAL);
-        assert_return(ret, -EINVAL);
-        assert_return(!bus_pid_changed(bus), -ECHILD);
+        bus_assert_return(bus, -EINVAL, error);
+        bus_assert_return(isempty(interface) || interface_name_is_valid(interface), -EINVAL, error);
+        bus_assert_return(member_name_is_valid(member), -EINVAL, error);
+        bus_assert_return(ret, -EINVAL, error);
+        bus_assert_return(!bus_pid_changed(bus), -ECHILD, error);
 
-        if (!BUS_IS_OPEN(bus->state))
-                return -ENOTCONN;
+        if (!BUS_IS_OPEN(bus->state)) {
+                r = -ENOTCONN;
+                goto fail;
+        }
 
         r = sd_bus_call_method(bus, destination, path, "org.freedesktop.DBus.Properties", "Get", error, &reply, "ss", strempty(interface), member);
         if (r < 0)
@@ -420,13 +444,16 @@ _public_ int sd_bus_get_property_strv(
 
         r = sd_bus_message_enter_container(reply, 'v', NULL);
         if (r < 0)
-                return r;
+                goto fail;
 
         r = sd_bus_message_read_strv(reply, ret);
         if (r < 0)
-                return r;
+                goto fail;
 
         return 0;
+
+fail:
+        return sd_bus_error_set_errno(error, r);
 }
 
 _public_ int sd_bus_set_property(
@@ -442,38 +469,43 @@ _public_ int sd_bus_set_property(
         va_list ap;
         int r;
 
-        assert_return(bus, -EINVAL);
-        assert_return(isempty(interface) || interface_name_is_valid(interface), -EINVAL);
-        assert_return(member_name_is_valid(member), -EINVAL);
-        assert_return(signature_is_single(type, false), -EINVAL);
-        assert_return(!bus_pid_changed(bus), -ECHILD);
+        bus_assert_return(bus, -EINVAL, error);
+        bus_assert_return(isempty(interface) || interface_name_is_valid(interface), -EINVAL, error);
+        bus_assert_return(member_name_is_valid(member), -EINVAL, error);
+        bus_assert_return(signature_is_single(type, false), -EINVAL, error);
+        bus_assert_return(!bus_pid_changed(bus), -ECHILD, error);
 
-        if (!BUS_IS_OPEN(bus->state))
-                return -ENOTCONN;
+        if (!BUS_IS_OPEN(bus->state)) {
+                r = -ENOTCONN;
+                goto fail;
+        }
 
         r = sd_bus_message_new_method_call(bus, &m, destination, path, "org.freedesktop.DBus.Properties", "Set");
         if (r < 0)
-                return r;
+                goto fail;
 
         r = sd_bus_message_append(m, "ss", strempty(interface), member);
         if (r < 0)
-                return r;
+                goto fail;
 
         r = sd_bus_message_open_container(m, 'v', type);
         if (r < 0)
-                return r;
+                goto fail;
 
         va_start(ap, type);
         r = bus_message_append_ap(m, type, ap);
         va_end(ap);
         if (r < 0)
-                return r;
+                goto fail;
 
         r = sd_bus_message_close_container(m);
         if (r < 0)
-                return r;
+                goto fail;
 
         return sd_bus_call(bus, m, 0, error, NULL);
+
+fail:
+        return sd_bus_error_set_errno(error, r);
 }
 
 _public_ int sd_bus_query_sender_creds(sd_bus_message *call, uint64_t mask, sd_bus_creds **creds) {

diff --git a/src/libsystemd/sd-bus/bus-internal.h b/src/libsystemd/sd-bus/bus-internal.h
index d420a7594f0faf66bed2fadeec458df5641c9125..9b68fdd64207e94bee436538839d3d05797bcfe8 100644 (file)
--- a/src/libsystemd/sd-bus/bus-internal.h
+++ b/src/libsystemd/sd-bus/bus-internal.h
@@ -419,3 +419,9 @@ int bus_maybe_reply_error(sd_bus_message *m, int r, sd_bus_error *error);
 
 bool is_kdbus_wanted(void);
 bool is_kdbus_available(void);
+
+#define bus_assert_return(expr, r, error)                               \
+        do {                                                            \
+                if (!assert_log(expr))                                  \
+                        return sd_bus_error_set_errno(error, r);        \
+        } while (false)

diff --git a/src/libsystemd/sd-bus/sd-bus.c b/src/libsystemd/sd-bus/sd-bus.c
index 763f830e2d68c37e95070118e837464e33bfbd27..b9e2782564bcbc2a9f42c13a472716fc356c9d61 100644 (file)
--- a/src/libsystemd/sd-bus/sd-bus.c
+++ b/src/libsystemd/sd-bus/sd-bus.c
@@ -1951,37 +1951,39 @@ _public_ int sd_bus_call(
         unsigned i;
         int r;
 
-        assert_return(m, -EINVAL);
-        assert_return(m->header->type == SD_BUS_MESSAGE_METHOD_CALL, -EINVAL);
-        assert_return(!(m->header->flags & BUS_MESSAGE_NO_REPLY_EXPECTED), -EINVAL);
-        assert_return(!bus_error_is_dirty(error), -EINVAL);
+        bus_assert_return(m, -EINVAL, error);
+        bus_assert_return(m->header->type == SD_BUS_MESSAGE_METHOD_CALL, -EINVAL, error);
+        bus_assert_return(!(m->header->flags & BUS_MESSAGE_NO_REPLY_EXPECTED), -EINVAL, error);
+        bus_assert_return(!bus_error_is_dirty(error), -EINVAL, error);
 
         if (!bus)
                 bus = m->bus;
 
-        assert_return(!bus_pid_changed(bus), -ECHILD);
-        assert_return(!bus->is_kernel || !(bus->hello_flags & KDBUS_HELLO_MONITOR), -EROFS);
+        bus_assert_return(!bus_pid_changed(bus), -ECHILD, error);
+        bus_assert_return(!bus->is_kernel || !(bus->hello_flags & KDBUS_HELLO_MONITOR), -EROFS, error);
 
-        if (!BUS_IS_OPEN(bus->state))
-                return -ENOTCONN;
+        if (!BUS_IS_OPEN(bus->state)) {
+                r = -ENOTCONN;
+                goto fail;
+        }
 
         r = bus_ensure_running(bus);
         if (r < 0)
-                return r;
+                goto fail;
 
         i = bus->rqueue_size;
 
         r = bus_seal_message(bus, m, usec);
         if (r < 0)
-                return r;
+                goto fail;
 
         r = bus_remarshal_message(bus, &m);
         if (r < 0)
-                return r;
+                goto fail;
 
         r = bus_send_internal(bus, m, &cookie, true);
         if (r < 0)
-                return r;
+                goto fail;
 
         timeout = calc_elapse(m->timeout);
 
@@ -2012,14 +2014,17 @@ _public_ int sd_bus_call(
                                         }
 
                                         r = sd_bus_error_setf(error, SD_BUS_ERROR_INCONSISTENT_MESSAGE, "Reply message contained file descriptors which I couldn't accept. Sorry.");
+                                        sd_bus_message_unref(incoming);
+                                        return r;
 
-                                } else if (incoming->header->type == SD_BUS_MESSAGE_METHOD_ERROR)
+                                } else if (incoming->header->type == SD_BUS_MESSAGE_METHOD_ERROR) {
                                         r = sd_bus_error_copy(error, &incoming->error);
-                                else
+                                        sd_bus_message_unref(incoming);
+                                        return r;
+                                } else {
                                         r = -EIO;
-
-                                sd_bus_message_unref(incoming);
-                                return r;
+                                        goto fail;
+                                }
 
                         } else if (BUS_MESSAGE_COOKIE(incoming) == cookie &&
                                    bus->unique_name &&
@@ -2035,7 +2040,8 @@ _public_ int sd_bus_call(
                                  * immediately. */
 
                                 sd_bus_message_unref(incoming);
-                                return -ELOOP;
+                                r = -ELOOP;
+                                goto fail;
                         }
 
                         /* Try to read more, right-away */
@@ -2046,10 +2052,10 @@ _public_ int sd_bus_call(
                 if (r < 0) {
                         if (r == -ENOTCONN || r == -ECONNRESET || r == -EPIPE || r == -ESHUTDOWN) {
                                 bus_enter_closing(bus);
-                                return -ECONNRESET;
+                                r = -ECONNRESET;
                         }
 
-                        return r;
+                        goto fail;
                 }
                 if (r > 0)
                         continue;
@@ -2058,8 +2064,10 @@ _public_ int sd_bus_call(
                         usec_t n;
 
                         n = now(CLOCK_MONOTONIC);
-                        if (n >= timeout)
-                                return -ETIMEDOUT;
+                        if (n >= timeout) {
+                                r = -ETIMEDOUT;
+                                goto fail;
+                        }
 
                         left = timeout - n;
                 } else
@@ -2067,20 +2075,25 @@ _public_ int sd_bus_call(
 
                 r = bus_poll(bus, true, left);
                 if (r < 0)
-                        return r;
-                if (r == 0)
-                        return -ETIMEDOUT;
+                        goto fail;
+                if (r == 0) {
+                        r = -ETIMEDOUT;
+                        goto fail;
+                }
 
                 r = dispatch_wqueue(bus);
                 if (r < 0) {
                         if (r == -ENOTCONN || r == -ECONNRESET || r == -EPIPE || r == -ESHUTDOWN) {
                                 bus_enter_closing(bus);
-                                return -ECONNRESET;
+                                r = -ECONNRESET;
                         }
 
-                        return r;
+                        goto fail;
                 }
         }
+
+fail:
+        return sd_bus_error_set_errno(error, r);
 }
 
 _public_ int sd_bus_get_fd(sd_bus *bus) {