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.

3 files changed, 135 insertions, 84 deletions

diff --git a/src/libsystemd/sd-bus/bus-convenience.c b/src/libsystemd/sd-bus/bus-convenience.c
index 87898449e6..79747e058b 100644
--- 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 d420a7594f..9b68fdd642 100644
--- 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 763f830e2d..b9e2782564 100644
--- 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) {