[PATCH 1/3] dbus-service: Allow property setters to return errors

[PATCH 1/3] dbus-service: Allow property setters to return errors

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 2954 bytes --]

Allow the property setter functions to synchronously return a message
pointer if they need to return an error.  On success complete still
needs to be called because the NULL return value couls also mean that
the setter will complete asynchronously.
---
 ell/dbus-service.c | 21 +++++++++++++++------
 ell/dbus-service.h |  2 +-
 2 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/ell/dbus-service.c b/ell/dbus-service.c
index 44baaaf..deffc07 100644
--- a/ell/dbus-service.c
+++ b/ell/dbus-service.c
@@ -1180,6 +1180,7 @@ static struct l_dbus_message *old_set_property(struct l_dbus *dbus,
 	const struct _dbus_property *property;
 	struct l_dbus_message_iter variant;
 	struct _dbus_object_tree *tree = _dbus_get_tree(dbus);
+	struct l_dbus_message *reply;
 
 	interface = l_hashmap_lookup(tree->interfaces,
 					l_dbus_message_get_interface(message));
@@ -1207,10 +1208,13 @@ static struct l_dbus_message *old_set_property(struct l_dbus *dbus,
 						"Property %s is read-only",
 						property_name);
 
-	property->setter(dbus, l_dbus_message_ref(message), &variant,
-				set_property_complete, user_data);
+	reply = property->setter(dbus, l_dbus_message_ref(message), &variant,
+					set_property_complete, user_data);
 
-	return NULL;
+	if (reply)
+		l_dbus_message_unref(message);
+
+	return reply;
 }
 
 static struct l_dbus_message *old_get_properties(struct l_dbus *dbus,
@@ -1729,6 +1733,7 @@ static struct l_dbus_message *properties_set(struct l_dbus *dbus,
 	struct l_dbus_message_iter variant;
 	struct _dbus_object_tree *tree = _dbus_get_tree(dbus);
 	const struct object_node *object;
+	struct l_dbus_message *reply;
 
 	if (!l_dbus_message_get_arguments(message, "ssv", &interface_name,
 						&property_name, &variant))
@@ -1774,10 +1779,14 @@ static struct l_dbus_message *properties_set(struct l_dbus *dbus,
 						"Object has no interface %s",
 						interface_name);
 
-	property->setter(dbus, l_dbus_message_ref(message), &variant,
-				properties_set_complete, instance->user_data);
+	reply = property->setter(dbus, l_dbus_message_ref(message), &variant,
+					properties_set_complete,
+					instance->user_data);
 
-	return NULL;
+	if (reply)
+		l_dbus_message_unref(message);
+
+	return reply;
 }
 
 static struct l_dbus_message *properties_get_all(struct l_dbus *dbus,
diff --git a/ell/dbus-service.h b/ell/dbus-service.h
index 2243b99..153bb3a 100644
--- a/ell/dbus-service.h
+++ b/ell/dbus-service.h
@@ -56,7 +56,7 @@ typedef void (*l_dbus_property_complete_cb_t) (struct l_dbus *,
 						struct l_dbus_message *,
 						struct l_dbus_message *error);
 
-typedef void (*l_dbus_property_set_cb_t) (struct l_dbus *,
+typedef struct l_dbus_message *(*l_dbus_property_set_cb_t) (struct l_dbus *,
 					struct l_dbus_message *message,
 					struct l_dbus_message_iter *new_value,
 					l_dbus_property_complete_cb_t complete,
-- 
2.7.4

[PATCH 2/3] examples: Update dbus-service example use of setter api

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 2589 bytes --]

---
 examples/dbus-service.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/examples/dbus-service.c b/examples/dbus-service.c
index 19b17a0..2a53ab8 100644
--- a/examples/dbus-service.c
+++ b/examples/dbus-service.c
@@ -106,27 +106,27 @@ static bool test_string_getter(struct l_dbus *dbus,
 	return true;
 }
 
-static void test_string_setter(struct l_dbus *dbus,
-				struct l_dbus_message *message,
-				struct l_dbus_message_iter *new_value,
-				l_dbus_property_complete_cb_t complete,
-				void *user_data)
+static struct l_dbus_message *test_string_setter(struct l_dbus *dbus,
+					struct l_dbus_message *message,
+					struct l_dbus_message_iter *new_value,
+					l_dbus_property_complete_cb_t complete,
+					void *user_data)
 {
 	const char *strvalue;
 	struct test_data *test = user_data;
 
-	if (!l_dbus_message_iter_get_variant(new_value, "s", &strvalue)) {
-		complete(dbus, message, l_dbus_message_new_error(message,
-					"org.test.InvalidArguments",
-					"String value expected"));
-		return;
-	}
+	if (!l_dbus_message_iter_get_variant(new_value, "s", &strvalue))
+		return l_dbus_message_new_error(message,
+						"org.test.InvalidArguments",
+						"String value expected");
 
 	l_info("New String value: %s", strvalue);
 	l_free(test->string);
 	test->string = l_strdup(strvalue);
 
 	complete(dbus, message, NULL);
+
+	return NULL;
 }
 
 static bool test_int_getter(struct l_dbus *dbus,
@@ -141,7 +141,7 @@ static bool test_int_getter(struct l_dbus *dbus,
 	return true;
 }
 
-static void test_int_setter(struct l_dbus *dbus,
+static struct l_dbus_message *test_int_setter(struct l_dbus *dbus,
 				struct l_dbus_message *message,
 				struct l_dbus_message_iter *new_value,
 				l_dbus_property_complete_cb_t complete,
@@ -150,17 +150,17 @@ static void test_int_setter(struct l_dbus *dbus,
 	uint32_t u;
 	struct test_data *test = user_data;
 
-	if (!l_dbus_message_iter_get_variant(new_value, "u", &u)) {
-		complete(dbus, message, l_dbus_message_new_error(message,
-					"org.test.InvalidArguments",
-					"Integer value expected"));
-		return;
-	}
+	if (!l_dbus_message_iter_get_variant(new_value, "u", &u))
+		return l_dbus_message_new_error(message,
+						"org.test.InvalidArguments",
+						"Integer value expected");
 
 	l_info("New Integer value: %u", u);
 	test->integer = u;
 
 	complete(dbus, message, NULL);
+
+	return NULL;
 }
 
 static void setup_test_interface(struct l_dbus_interface *interface)
-- 
2.7.4

[PATCH 3/3] unit: Update the uses of dbus property setter api

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 3568 bytes --]

---
 unit/test-dbus-properties.c | 53 +++++++++++++++++++++++++++------------------
 unit/test-dbus-service.c    |  3 ++-
 2 files changed, 34 insertions(+), 22 deletions(-)

diff --git a/unit/test-dbus-properties.c b/unit/test-dbus-properties.c
index b11d1d5..08916b3 100644
--- a/unit/test-dbus-properties.c
+++ b/unit/test-dbus-properties.c
@@ -220,20 +220,26 @@ static bool test_string_getter(struct l_dbus *dbus,
 static bool setter_called;
 static bool int_optional;
 
-static void test_string_setter(struct l_dbus *dbus,
-				struct l_dbus_message *message,
-				struct l_dbus_message_iter *new_value,
-				l_dbus_property_complete_cb_t complete,
-				void *user_data)
+static struct l_dbus_message *test_string_setter(struct l_dbus *dbus,
+					struct l_dbus_message *message,
+					struct l_dbus_message_iter *new_value,
+					l_dbus_property_complete_cb_t complete,
+					void *user_data)
 {
 	const char *strvalue;
 
-	test_assert(l_dbus_message_iter_get_variant(new_value, "s", &strvalue));
-	test_assert(!strcmp(strvalue, "bar"));
+	if (!l_dbus_message_iter_get_variant(new_value, "s", &strvalue))
+		goto done;
+
+	if (strcmp(strvalue, "bar"))
+		goto done;
 
 	setter_called = true;
 
+done:
 	complete(dbus, message, NULL);
+
+	return NULL;
 }
 
 static bool test_int_getter(struct l_dbus *dbus,
@@ -251,32 +257,37 @@ static bool test_int_getter(struct l_dbus *dbus,
 	return l_dbus_message_builder_append_basic(builder, 'u', &u);
 }
 
-static void test_int_setter(struct l_dbus *dbus,
-				struct l_dbus_message *message,
-				struct l_dbus_message_iter *new_value,
-				l_dbus_property_complete_cb_t complete,
-				void *user_data)
+static struct l_dbus_message *test_int_setter(struct l_dbus *dbus,
+					struct l_dbus_message *message,
+					struct l_dbus_message_iter *new_value,
+					l_dbus_property_complete_cb_t complete,
+					void *user_data)
 {
 	uint32_t u;
 
-	test_assert(l_dbus_message_iter_get_variant(new_value, "u", &u));
-	test_assert(u == 42);
+	if (!l_dbus_message_iter_get_variant(new_value, "u", &u))
+		goto done;
+
+	if (u != 42)
+		goto done;
 
 	setter_called = true;
 
+done:
 	complete(dbus, message, NULL);
+
+	return NULL;
 }
 
-static void test_error_setter(struct l_dbus *dbus,
-				struct l_dbus_message *message,
-				struct l_dbus_message_iter *new_value,
-				l_dbus_property_complete_cb_t complete,
-				void *user_data)
+static struct l_dbus_message *test_error_setter(struct l_dbus *dbus,
+					struct l_dbus_message *message,
+					struct l_dbus_message_iter *new_value,
+					l_dbus_property_complete_cb_t complete,
+					void *user_data)
 {
 	setter_called = true;
 
-	complete(dbus, message, l_dbus_message_new_error(message,
-						"org.test.Error", "Error"));
+	return l_dbus_message_new_error(message, "org.test.Error", "Error");
 }
 
 static bool test_path_getter(struct l_dbus *dbus,
diff --git a/unit/test-dbus-service.c b/unit/test-dbus-service.c
index 8a395b1..c27869b 100644
--- a/unit/test-dbus-service.c
+++ b/unit/test-dbus-service.c
@@ -399,12 +399,13 @@ static bool test_property_getter(struct l_dbus *dbus,
 	return true;
 }
 
-static void test_property_setter(struct l_dbus *dbus,
+static struct l_dbus_message *test_property_setter(struct l_dbus *dbus,
 					struct l_dbus_message *message,
 					struct l_dbus_message_iter *new_value,
 					l_dbus_property_complete_cb_t complete,
 					void *user_data)
 {
+	return NULL;
 }
 
 int main(int argc, char *argv[])
-- 
2.7.4

Re: [PATCH 1/3] dbus-service: Allow property setters to return errors

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 525 bytes --]

Hi Andrew,

On 07/08/2016 10:55 AM, Andrew Zaborowski wrote:
> Allow the property setter functions to synchronously return a message
> pointer if they need to return an error.  On success complete still
> needs to be called because the NULL return value couls also mean that
> the setter will complete asynchronously.
> ---
>   ell/dbus-service.c | 21 +++++++++++++++------
>   ell/dbus-service.h |  2 +-
>   2 files changed, 16 insertions(+), 7 deletions(-)
>

All three applied, thanks.

Regards,
-Denis