Hi,
the attached patchset implement the org.freedesktop.DBus.Properties.Get() interface for properties of primitive types and arrays of primitive types (except for array of bools as DBus bool is of different size than C-sized bool).
Getters are always synchronous, mostly to allow sane GetAll processing. The getters also never error out, but rather return a default value.
The first two patches add utility functions, one by me and one by Pavel that make it easier to return a primitive type or an array in a variant. I wish the DBus library itself had a helper like this...
Patch 3/6 just allows the messages to be passed through to the IFP responder.
The whole patchset is based on Stef's WIP branch and expands on it, that's patch 4/6. There is a setter stub that, while not finished, is not harmful either and can be expanded on later.
Patch 5/6 is the meat of the whole patchset. It implements the sbus codegen to generate getter functions based on the XML description of the interface and adds unit test for all the supported types and all supported arrays. Pavel was kind enough to contribute several bugfixes for this patch -- thank you!
Patch 6/6 is a bugfix by Pavel, one that I agree with, you can consider the patch acked by me.
The next patch I'm going to send will implement a 'raw property getter' that would allow the interface developer to return complex values for a property value.
On 05/12/2014 11:27 PM, Jakub Hrozek wrote:
Hi,
the attached patchset implement the org.freedesktop.DBus.Properties.Get() interface for properties of primitive types and arrays of primitive types (except for array of bools as DBus bool is of different size than C-sized bool).
Getters are always synchronous, mostly to allow sane GetAll processing. The getters also never error out, but rather return a default value.
The first two patches add utility functions, one by me and one by Pavel that make it easier to return a primitive type or an array in a variant. I wish the DBus library itself had a helper like this...
Patch 3/6 just allows the messages to be passed through to the IFP responder.
The whole patchset is based on Stef's WIP branch and expands on it, that's patch 4/6. There is a setter stub that, while not finished, is not harmful either and can be expanded on later.
Patch 5/6 is the meat of the whole patchset. It implements the sbus codegen to generate getter functions based on the XML description of the interface and adds unit test for all the supported types and all supported arrays. Pavel was kind enough to contribute several bugfixes for this patch -- thank you!
Patch 6/6 is a bugfix by Pavel, one that I agree with, you can consider the patch acked by me.
The next patch I'm going to send will implement a 'raw property getter' that would allow the interface developer to return complex values for a property value.
Ack. Nice work.
On (12/05/14 23:27), Jakub Hrozek wrote:
Hi,
the attached patchset implement the org.freedesktop.DBus.Properties.Get() interface for properties of primitive types and arrays of primitive types (except for array of bools as DBus bool is of different size than C-sized bool).
Getters are always synchronous, mostly to allow sane GetAll processing. The getters also never error out, but rather return a default value.
The first two patches add utility functions, one by me and one by Pavel that make it easier to return a primitive type or an array in a variant. I wish the DBus library itself had a helper like this...
Patch 3/6 just allows the messages to be passed through to the IFP responder.
The whole patchset is based on Stef's WIP branch and expands on it, that's patch 4/6. There is a setter stub that, while not finished, is not harmful either and can be expanded on later.
Patch 5/6 is the meat of the whole patchset. It implements the sbus codegen to generate getter functions based on the XML description of the interface and adds unit test for all the supported types and all supported arrays. Pavel was kind enough to contribute several bugfixes for this patch -- thank you!
Patch 6/6 is a bugfix by Pavel, one that I agree with, you can consider the patch acked by me.
The next patch I'm going to send will implement a 'raw property getter' that would allow the interface developer to return complex values for a property value.
From f5c17fe154cc396f7a497dcc389c8a2fd5037a7d Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Mon, 12 May 2014 18:59:35 +0200 Subject: [PATCH 1/6] SBUS: Utility function sbus_request_return_as_variant
Adds a utility function that returns a single value with a given type in a variant. This utility function will be used by the Get property call.
src/sbus/sssd_dbus.h | 4 ++++ src/sbus/sssd_dbus_request.c | 57 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+)
diff --git a/src/sbus/sssd_dbus.h b/src/sbus/sssd_dbus.h index 15ca521184f82c7424549907b7afa912f891c3ad..efb0296813e9946f2b2fa1fd867592315cc4ee4f 100644 --- a/src/sbus/sssd_dbus.h +++ b/src/sbus/sssd_dbus.h @@ -246,6 +246,10 @@ int sbus_request_return_and_finish(struct sbus_request *dbus_req, int first_arg_type, ...);
+int sbus_request_return_as_variant(struct sbus_request *dbus_req,
int type,const void *value);/*
- Return an error for a DBus method call request. The @error is a normal
- DBusError.
diff --git a/src/sbus/sssd_dbus_request.c b/src/sbus/sssd_dbus_request.c index 2d23931aa07a83c15c30b31615d5db7cfee11a0a..af439b284024b2b335adc0119f131dd6046c61bc 100644 --- a/src/sbus/sssd_dbus_request.c +++ b/src/sbus/sssd_dbus_request.c @@ -125,6 +125,63 @@ int sbus_request_return_and_finish(struct sbus_request *dbus_req, return ret; }
+int sbus_request_return_as_variant(struct sbus_request *dbus_req,
int type,const void *value)+{
- DBusMessage *reply;
- dbus_bool_t dbret;
- DBusMessageIter iter;
- DBusMessageIter valiter;
- char strtype[2];
- int ret;
- snprintf(strtype, 2, "%c", type);
- reply = dbus_message_new_method_return(dbus_req->message);
- if (reply == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "Out of memory allocating DBus message\n");sbus_request_finish(dbus_req, NULL);return ENOMEM;- }
- dbus_message_iter_init_append(reply, &iter);
- dbret = dbus_message_iter_open_container(&iter, DBUS_TYPE_VARIANT,
strtype, &valiter);- if (!dbret) {
sbus_request_finish(dbus_req, NULL);ret = EINVAL;goto done;- }
- dbret = dbus_message_iter_append_basic(&valiter, type, value);
- if (!dbret) {
sbus_request_finish(dbus_req, NULL);ret = EINVAL;goto done;- }
- dbret = dbus_message_iter_close_container(&iter, &valiter);
- if (!dbret) {
sbus_request_finish(dbus_req, NULL);ret = EINVAL;goto done;- }
// dbret will be 0 after this line
- if (dbret) {
ret = sbus_request_finish(dbus_req, reply);- } else {
DEBUG(SSSDBG_CRIT_FAILURE, "Couldn't build DBus message\n");sbus_request_finish(dbus_req, NULL);ret = EINVAL;
// this branch is dead code
- }
+done:
- dbus_message_unref(reply);
- return ret;
+}
int sbus_request_fail_and_finish(struct sbus_request *dbus_req, const DBusError *error) { -- 1.9.0
From a0fe9eb748301ed5df475ff55e375ba2bcd75f31 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Mon, 12 May 2014 19:00:36 +0200 Subject: [PATCH 2/6] SBUS: Utility function sbus_request_return_array_as_variant
Adds a utility function that returns an array of types values, each of a given size, with a given type in a variant. This utility function will be used by the GetAll property call.
src/sbus/sssd_dbus.h | 6 +++ src/sbus/sssd_dbus_request.c | 103 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 109 insertions(+)
diff --git a/src/sbus/sssd_dbus_request.c b/src/sbus/sssd_dbus_request.c index af439b284024b2b335adc0119f131dd6046c61bc..f2870077d6dc96b94c083ccdc9fc9a4c00c0428d 100644 --- a/src/sbus/sssd_dbus_request.c +++ b/src/sbus/sssd_dbus_request.c @@ -182,6 +182,109 @@ done: } +int sbus_request_return_array_as_variant(struct sbus_request *dbus_req,
int type,uint8_t *values,const int len,const size_t item_size)+{
// snip
- dbret = dbus_message_iter_close_container(&iter, &variant_iter);
- if (!dbret) {
sbus_request_finish(dbus_req, NULL);ret = EINVAL;goto done;- }
// dbret will be 0 after this line
- if (dbret) {
ret = sbus_request_finish(dbus_req, reply);- } else {
DEBUG(SSSDBG_CRIT_FAILURE, "Couldn't build DBus message\n");sbus_request_finish(dbus_req, NULL);ret = EINVAL;
// the same problem as in 1st patch.
- }
+done:
- if (reply != NULL) {
dbus_message_unref(reply);- }
- talloc_free(tmp_ctx);
- return ret;
+}
int sbus_request_fail_and_finish(struct sbus_request *dbus_req, const DBusError *error) { -- 1.9.0
From 5733ceac904342a7a979d0efe066afd9f093ebe9 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Tue, 22 Apr 2014 21:50:28 +0200 Subject: [PATCH 3/6] IFP: Allow Set, Get and GetAll from DBus.Properties
The InfoPipe will support all three of: DBus.Properties.Get DBus.Properties.GetAll DBus.Properties.Set
Hence it must allow these calls to be received.
src/responder/ifp/org.freedesktop.sssd.infopipe.conf | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/src/responder/ifp/org.freedesktop.sssd.infopipe.conf b/src/responder/ifp/org.freedesktop.sssd.infopipe.conf index 56e460aa11c2534e95092fb10216b05c50c04d59..4f2a906ba812d46b6bca401d3fc14baa5c18de50 100644 --- a/src/responder/ifp/org.freedesktop.sssd.infopipe.conf +++ b/src/responder/ifp/org.freedesktop.sssd.infopipe.conf @@ -19,6 +19,16 @@ <allow send_destination="org.freedesktop.sssd.infopipe" send_interface="org.freedesktop.DBus.Introspectable"/>
- <allow send_destination="org.freedesktop.sssd.infopipe"
send_interface="org.freedesktop.DBus.Properties"send_member="GetAll"/>- <allow send_destination="org.freedesktop.sssd.infopipe"
send_interface="org.freedesktop.DBus.Properties"send_member="Get"/>- <allow send_destination="org.freedesktop.sssd.infopipe"
send_interface="org.freedesktop.DBus.Properties"send_member="Set"/><allow send_interface="org.freedesktop.sssd.infopipe"/> </policy>
-- 1.9.0
From d23886c484bc955bf4bc03998601aeec1650ce02 Mon Sep 17 00:00:00 2001 From: Stef Walter stefw@redhat.com Date: Tue, 25 Feb 2014 18:31:03 +0100 Subject: [PATCH 4/6] WIP properties
I knot that is is a Stef's patch, but it does not look professional to have WIP patch in master.
From 796d392b6cb33d6ded6ae58e1abcca84cf6a9b25 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Wed, 23 Apr 2014 03:01:21 +0200 Subject: [PATCH 5/6] SBUS: Implement org.freedesktop.DBus.Properties.Get for primitive types
This patch implements type-safe getters for primitive types and their arrays. The patch includes unit tests of all supported types and arrays of these types.
All getter are synchronous. The getters never fail, instead, they return a default or 'not defined' value. Making the getters synchronous and always returning a value will make it significantly easier to implement the GetAll method.
src/sbus/sbus_codegen | 114 ++++--- src/sbus/sssd_dbus.h | 6 + src/sbus/sssd_dbus_meta.h | 2 + src/sbus/sssd_dbus_properties.c | 75 +++++ src/tests/sbus_codegen_tests.c | 490 ++++++++++++++++++++++++++++- src/tests/sbus_codegen_tests.xml | 26 ++ src/tests/sbus_codegen_tests_generated.c | 522 +++++++++++++++++++++++++++++++ src/tests/sbus_codegen_tests_generated.h | 44 +++ 8 files changed, 1235 insertions(+), 44 deletions(-)
//snip
diff --git a/src/sbus/sssd_dbus_properties.c b/src/sbus/sssd_dbus_properties.c index 835b3078b5abd124ab98265401aa48533a306ee3..bdd5b432fe57ca66a4d06f98720578b3a175a65c 100644 --- a/src/sbus/sssd_dbus_properties.c +++ b/src/sbus/sssd_dbus_properties.c @@ -105,6 +105,77 @@ dispatch_properties_set(struct sbus_connection *conn, return EOK; }
+static int +dispatch_properties_get(struct sbus_connection *conn,
struct sbus_interface *intf,DBusMessage *message)+{
- struct sbus_request *req;
- const char *signature;
- const struct sbus_interface_meta *meta;
- DBusMessageIter iter;
- sbus_msg_handler_fn handler_fn;
- const struct sbus_property_meta *property;
- const char *interface_name;
- const char *property_name;
- req = sbus_new_request(conn, intf, message);
- if (req == NULL) {
return ENOMEM;- }
- meta = intf->vtable->meta;
- signature = dbus_message_get_signature(message);
- /* Interface name, property name */
- if (strcmp(signature, "ss") != 0) {
return sbus_request_fail_and_finish(req,sbus_error_new(req,DBUS_ERROR_INVALID_ARGS,"Invalid argument types passed to Get method"));- }
- dbus_message_iter_init(message, &iter);
- dbus_message_iter_get_basic(&iter, &interface_name);
- dbus_message_iter_next(&iter);
- dbus_message_iter_get_basic(&iter, &property_name);
- if (strcmp(interface_name, meta->name) != 0) {
return sbus_request_fail_and_finish(req,sbus_error_new(req,DBUS_ERROR_UNKNOWN_INTERFACE,"No such interface"));- }
- property = sbus_meta_find_property(intf->vtable->meta, property_name);
- if (property == NULL) {
return sbus_request_fail_and_finish(req,sbus_error_new(req,DBUS_ERROR_UNKNOWN_PROPERTY,"No such property"));- }
- if (!(property->flags & SBUS_PROPERTY_READABLE)) {
return sbus_request_fail_and_finish(req,sbus_error_new(req,DBUS_ERROR_ACCESS_DENIED,"Property is not readable"));- }
- handler_fn = VTABLE_FUNC(intf->vtable, property->vtable_offset_get);
- if (!handler_fn) {
return sbus_request_fail_and_finish(req,sbus_error_new(req,DBUS_ERROR_NOT_SUPPORTED,"Not implemented"));- }
- sbus_request_invoke_or_finish(req, handler_fn,
intf->instance_data,property->invoker_get);- return EOK;
+}
int sbus_properties_dispatch(struct sbus_request *dbus_req) { const char *member; @@ -116,6 +187,10 @@ int sbus_properties_dispatch(struct sbus_request *dbus_req) return dispatch_properties_set(dbus_req->conn, dbus_req->intf, dbus_req->message);
- } else if (strcmp (member, "Get") == 0) {
return dispatch_properties_get(dbus_req->conn,dbus_req->intf,dbus_req->message);
The last two patches break build on rhel6.
src/sbus/sssd_dbus_properties.c:66: error: 'DBUS_ERROR_UNKNOWN_INTERFACE' undeclared (first use in this function) src/sbus/sssd_dbus_properties.c:66: error: (Each undeclared identifier is reported only once src/sbus/sssd_dbus_properties.c:66: error: for each function it appears in.) src/sbus/sssd_dbus_properties.c:74: error: 'DBUS_ERROR_UNKNOWN_PROPERTY' undeclared (first use in this function) src/sbus/sssd_dbus_properties.c:81: error: 'DBUS_ERROR_PROPERTY_READ_ONLY' undeclared (first use in this function) src/sbus/sssd_dbus_properties.c: In function 'dispatch_properties_get': src/sbus/sssd_dbus_properties.c:146: error: 'DBUS_ERROR_UNKNOWN_INTERFACE' undeclared (first use in this function) src/sbus/sssd_dbus_properties.c:154: error: 'DBUS_ERROR_UNKNOWN_PROPERTY' undeclared (first use in this function) make[2]: *** [src/sbus/libsss_util_la-sssd_dbus_properties.lo] Error 1
LS
On 13.05.2014 18:04, Lukas Slebodnik wrote:
On (12/05/14 23:27), Jakub Hrozek wrote:
Hi,
the attached patchset implement the org.freedesktop.DBus.Properties.Get() interface for properties of primitive types and arrays of primitive types (except for array of bools as DBus bool is of different size than C-sized bool).
Getters are always synchronous, mostly to allow sane GetAll processing. The getters also never error out, but rather return a default value.
The first two patches add utility functions, one by me and one by Pavel that make it easier to return a primitive type or an array in a variant. I wish the DBus library itself had a helper like this...
Patch 3/6 just allows the messages to be passed through to the IFP responder.
The whole patchset is based on Stef's WIP branch and expands on it, that's patch 4/6. There is a setter stub that, while not finished, is not harmful either and can be expanded on later.
Patch 5/6 is the meat of the whole patchset. It implements the sbus codegen to generate getter functions based on the XML description of the interface and adds unit test for all the supported types and all supported arrays. Pavel was kind enough to contribute several bugfixes for this patch -- thank you!
Patch 6/6 is a bugfix by Pavel, one that I agree with, you can consider the patch acked by me.
The next patch I'm going to send will implement a 'raw property getter' that would allow the interface developer to return complex values for a property value.
From f5c17fe154cc396f7a497dcc389c8a2fd5037a7d Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Mon, 12 May 2014 18:59:35 +0200 Subject: [PATCH 1/6] SBUS: Utility function sbus_request_return_as_variant
Adds a utility function that returns a single value with a given type in a variant. This utility function will be used by the Get property call.
src/sbus/sssd_dbus.h | 4 ++++ src/sbus/sssd_dbus_request.c | 57 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+)
diff --git a/src/sbus/sssd_dbus.h b/src/sbus/sssd_dbus.h index 15ca521184f82c7424549907b7afa912f891c3ad..efb0296813e9946f2b2fa1fd867592315cc4ee4f 100644 --- a/src/sbus/sssd_dbus.h +++ b/src/sbus/sssd_dbus.h @@ -246,6 +246,10 @@ int sbus_request_return_and_finish(struct sbus_request *dbus_req, int first_arg_type, ...);
+int sbus_request_return_as_variant(struct sbus_request *dbus_req,
int type,const void *value);/*
- Return an error for a DBus method call request. The @error is a normal
- DBusError.
diff --git a/src/sbus/sssd_dbus_request.c b/src/sbus/sssd_dbus_request.c index 2d23931aa07a83c15c30b31615d5db7cfee11a0a..af439b284024b2b335adc0119f131dd6046c61bc 100644 --- a/src/sbus/sssd_dbus_request.c +++ b/src/sbus/sssd_dbus_request.c @@ -125,6 +125,63 @@ int sbus_request_return_and_finish(struct sbus_request *dbus_req, return ret; }
+int sbus_request_return_as_variant(struct sbus_request *dbus_req,
int type,const void *value)+{
- DBusMessage *reply;
- dbus_bool_t dbret;
- DBusMessageIter iter;
- DBusMessageIter valiter;
- char strtype[2];
- int ret;
- snprintf(strtype, 2, "%c", type);
- reply = dbus_message_new_method_return(dbus_req->message);
- if (reply == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "Out of memory allocating DBus message\n");sbus_request_finish(dbus_req, NULL);return ENOMEM;- }
- dbus_message_iter_init_append(reply, &iter);
- dbret = dbus_message_iter_open_container(&iter, DBUS_TYPE_VARIANT,
strtype, &valiter);- if (!dbret) {
sbus_request_finish(dbus_req, NULL);ret = EINVAL;goto done;- }
- dbret = dbus_message_iter_append_basic(&valiter, type, value);
- if (!dbret) {
sbus_request_finish(dbus_req, NULL);ret = EINVAL;goto done;- }
- dbret = dbus_message_iter_close_container(&iter, &valiter);
- if (!dbret) {
sbus_request_finish(dbus_req, NULL);ret = EINVAL;goto done;- }
// dbret will be 0 after this line
- if (dbret) {
ret = sbus_request_finish(dbus_req, reply);- } else {
DEBUG(SSSDBG_CRIT_FAILURE, "Couldn't build DBus message\n");sbus_request_finish(dbus_req, NULL);ret = EINVAL;// this branch is dead code
- }
+done:
- dbus_message_unref(reply);
- return ret;
+}
int sbus_request_fail_and_finish(struct sbus_request *dbus_req, const DBusError *error) { -- 1.9.0
From a0fe9eb748301ed5df475ff55e375ba2bcd75f31 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Mon, 12 May 2014 19:00:36 +0200 Subject: [PATCH 2/6] SBUS: Utility function sbus_request_return_array_as_variant
Adds a utility function that returns an array of types values, each of a given size, with a given type in a variant. This utility function will be used by the GetAll property call.
src/sbus/sssd_dbus.h | 6 +++ src/sbus/sssd_dbus_request.c | 103 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 109 insertions(+)
diff --git a/src/sbus/sssd_dbus_request.c b/src/sbus/sssd_dbus_request.c index af439b284024b2b335adc0119f131dd6046c61bc..f2870077d6dc96b94c083ccdc9fc9a4c00c0428d 100644 --- a/src/sbus/sssd_dbus_request.c +++ b/src/sbus/sssd_dbus_request.c @@ -182,6 +182,109 @@ done: } +int sbus_request_return_array_as_variant(struct sbus_request *dbus_req,
int type,uint8_t *values,const int len,const size_t item_size)+{
// snip
- dbret = dbus_message_iter_close_container(&iter, &variant_iter);
- if (!dbret) {
sbus_request_finish(dbus_req, NULL);ret = EINVAL;goto done;- }
// dbret will be 0 after this line
- if (dbret) {
ret = sbus_request_finish(dbus_req, reply);- } else {
DEBUG(SSSDBG_CRIT_FAILURE, "Couldn't build DBus message\n");sbus_request_finish(dbus_req, NULL);ret = EINVAL;// the same problem as in 1st patch.
- }
+done:
- if (reply != NULL) {
dbus_message_unref(reply);- }
- talloc_free(tmp_ctx);
- return ret;
+}
int sbus_request_fail_and_finish(struct sbus_request *dbus_req, const DBusError *error) { -- 1.9.0
From 5733ceac904342a7a979d0efe066afd9f093ebe9 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Tue, 22 Apr 2014 21:50:28 +0200 Subject: [PATCH 3/6] IFP: Allow Set, Get and GetAll from DBus.Properties
The InfoPipe will support all three of: DBus.Properties.Get DBus.Properties.GetAll DBus.Properties.Set
Hence it must allow these calls to be received.
src/responder/ifp/org.freedesktop.sssd.infopipe.conf | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/src/responder/ifp/org.freedesktop.sssd.infopipe.conf b/src/responder/ifp/org.freedesktop.sssd.infopipe.conf index 56e460aa11c2534e95092fb10216b05c50c04d59..4f2a906ba812d46b6bca401d3fc14baa5c18de50 100644 --- a/src/responder/ifp/org.freedesktop.sssd.infopipe.conf +++ b/src/responder/ifp/org.freedesktop.sssd.infopipe.conf @@ -19,6 +19,16 @@ <allow send_destination="org.freedesktop.sssd.infopipe" send_interface="org.freedesktop.DBus.Introspectable"/>
- <allow send_destination="org.freedesktop.sssd.infopipe"
send_interface="org.freedesktop.DBus.Properties"send_member="GetAll"/>- <allow send_destination="org.freedesktop.sssd.infopipe"
send_interface="org.freedesktop.DBus.Properties"send_member="Get"/>- <allow send_destination="org.freedesktop.sssd.infopipe"
send_interface="org.freedesktop.DBus.Properties"send_member="Set"/><allow send_interface="org.freedesktop.sssd.infopipe"/> </policy>
-- 1.9.0
From d23886c484bc955bf4bc03998601aeec1650ce02 Mon Sep 17 00:00:00 2001 From: Stef Walter stefw@redhat.com Date: Tue, 25 Feb 2014 18:31:03 +0100 Subject: [PATCH 4/6] WIP properties
I knot that is is a Stef's patch, but it does not look professional to have WIP patch in master.
From 796d392b6cb33d6ded6ae58e1abcca84cf6a9b25 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Wed, 23 Apr 2014 03:01:21 +0200 Subject: [PATCH 5/6] SBUS: Implement org.freedesktop.DBus.Properties.Get for primitive types
This patch implements type-safe getters for primitive types and their arrays. The patch includes unit tests of all supported types and arrays of these types.
All getter are synchronous. The getters never fail, instead, they return a default or 'not defined' value. Making the getters synchronous and always returning a value will make it significantly easier to implement the GetAll method.
src/sbus/sbus_codegen | 114 ++++--- src/sbus/sssd_dbus.h | 6 + src/sbus/sssd_dbus_meta.h | 2 + src/sbus/sssd_dbus_properties.c | 75 +++++ src/tests/sbus_codegen_tests.c | 490 ++++++++++++++++++++++++++++- src/tests/sbus_codegen_tests.xml | 26 ++ src/tests/sbus_codegen_tests_generated.c | 522 +++++++++++++++++++++++++++++++ src/tests/sbus_codegen_tests_generated.h | 44 +++ 8 files changed, 1235 insertions(+), 44 deletions(-)
//snip
diff --git a/src/sbus/sssd_dbus_properties.c b/src/sbus/sssd_dbus_properties.c index 835b3078b5abd124ab98265401aa48533a306ee3..bdd5b432fe57ca66a4d06f98720578b3a175a65c 100644 --- a/src/sbus/sssd_dbus_properties.c +++ b/src/sbus/sssd_dbus_properties.c @@ -105,6 +105,77 @@ dispatch_properties_set(struct sbus_connection *conn, return EOK; }
+static int +dispatch_properties_get(struct sbus_connection *conn,
struct sbus_interface *intf,DBusMessage *message)+{
- struct sbus_request *req;
- const char *signature;
- const struct sbus_interface_meta *meta;
- DBusMessageIter iter;
- sbus_msg_handler_fn handler_fn;
- const struct sbus_property_meta *property;
- const char *interface_name;
- const char *property_name;
- req = sbus_new_request(conn, intf, message);
- if (req == NULL) {
return ENOMEM;- }
- meta = intf->vtable->meta;
- signature = dbus_message_get_signature(message);
- /* Interface name, property name */
- if (strcmp(signature, "ss") != 0) {
return sbus_request_fail_and_finish(req,sbus_error_new(req,DBUS_ERROR_INVALID_ARGS,"Invalid argument types passed to Get method"));- }
- dbus_message_iter_init(message, &iter);
- dbus_message_iter_get_basic(&iter, &interface_name);
- dbus_message_iter_next(&iter);
- dbus_message_iter_get_basic(&iter, &property_name);
- if (strcmp(interface_name, meta->name) != 0) {
return sbus_request_fail_and_finish(req,sbus_error_new(req,DBUS_ERROR_UNKNOWN_INTERFACE,"No such interface"));- }
- property = sbus_meta_find_property(intf->vtable->meta, property_name);
- if (property == NULL) {
return sbus_request_fail_and_finish(req,sbus_error_new(req,DBUS_ERROR_UNKNOWN_PROPERTY,"No such property"));- }
- if (!(property->flags & SBUS_PROPERTY_READABLE)) {
return sbus_request_fail_and_finish(req,sbus_error_new(req,DBUS_ERROR_ACCESS_DENIED,"Property is not readable"));- }
- handler_fn = VTABLE_FUNC(intf->vtable, property->vtable_offset_get);
- if (!handler_fn) {
return sbus_request_fail_and_finish(req,sbus_error_new(req,DBUS_ERROR_NOT_SUPPORTED,"Not implemented"));- }
- sbus_request_invoke_or_finish(req, handler_fn,
intf->instance_data,property->invoker_get);- return EOK;
+}
int sbus_properties_dispatch(struct sbus_request *dbus_req) { const char *member; @@ -116,6 +187,10 @@ int sbus_properties_dispatch(struct sbus_request *dbus_req) return dispatch_properties_set(dbus_req->conn, dbus_req->intf, dbus_req->message);
- } else if (strcmp (member, "Get") == 0) {
return dispatch_properties_get(dbus_req->conn,dbus_req->intf,dbus_req->message);The last two patches break build on rhel6.
src/sbus/sssd_dbus_properties.c:66: error: 'DBUS_ERROR_UNKNOWN_INTERFACE' undeclared (first use in this function) src/sbus/sssd_dbus_properties.c:66: error: (Each undeclared identifier is reported only once src/sbus/sssd_dbus_properties.c:66: error: for each function it appears in.) src/sbus/sssd_dbus_properties.c:74: error: 'DBUS_ERROR_UNKNOWN_PROPERTY' undeclared (first use in this function) src/sbus/sssd_dbus_properties.c:81: error: 'DBUS_ERROR_PROPERTY_READ_ONLY' undeclared (first use in this function) src/sbus/sssd_dbus_properties.c: In function 'dispatch_properties_get': src/sbus/sssd_dbus_properties.c:146: error: 'DBUS_ERROR_UNKNOWN_INTERFACE' undeclared (first use in this function) src/sbus/sssd_dbus_properties.c:154: error: 'DBUS_ERROR_UNKNOWN_PROPERTY' undeclared (first use in this function) make[2]: *** [src/sbus/libsss_util_la-sssd_dbus_properties.lo] Error 1
Yes, these errors and related constants were a late addition to DBus. When implementing a service, we should #define them when not present.
By the way, if we ever *call* an external services, we should handle org.freedesktop.DBus.Error.UnknownMethod (ie: DBUS_ERROR_UNKNOWN_METHOD) as a stand-in for UnknownObject, UnknownInterface, UnknownProperty if the service returns it.
Stef
On Tue, May 13, 2014 at 08:42:19PM +0200, Stef Walter wrote:
On 13.05.2014 18:04, Lukas Slebodnik wrote:
On (12/05/14 23:27), Jakub Hrozek wrote:
Hi,
the attached patchset implement the org.freedesktop.DBus.Properties.Get() interface for properties of primitive types and arrays of primitive types (except for array of bools as DBus bool is of different size than C-sized bool).
Getters are always synchronous, mostly to allow sane GetAll processing. The getters also never error out, but rather return a default value.
The first two patches add utility functions, one by me and one by Pavel that make it easier to return a primitive type or an array in a variant. I wish the DBus library itself had a helper like this...
Patch 3/6 just allows the messages to be passed through to the IFP responder.
The whole patchset is based on Stef's WIP branch and expands on it, that's patch 4/6. There is a setter stub that, while not finished, is not harmful either and can be expanded on later.
Patch 5/6 is the meat of the whole patchset. It implements the sbus codegen to generate getter functions based on the XML description of the interface and adds unit test for all the supported types and all supported arrays. Pavel was kind enough to contribute several bugfixes for this patch -- thank you!
Patch 6/6 is a bugfix by Pavel, one that I agree with, you can consider the patch acked by me.
The next patch I'm going to send will implement a 'raw property getter' that would allow the interface developer to return complex values for a property value.
From f5c17fe154cc396f7a497dcc389c8a2fd5037a7d Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Mon, 12 May 2014 18:59:35 +0200 Subject: [PATCH 1/6] SBUS: Utility function sbus_request_return_as_variant
Adds a utility function that returns a single value with a given type in a variant. This utility function will be used by the Get property call.
src/sbus/sssd_dbus.h | 4 ++++ src/sbus/sssd_dbus_request.c | 57 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+)
diff --git a/src/sbus/sssd_dbus.h b/src/sbus/sssd_dbus.h index 15ca521184f82c7424549907b7afa912f891c3ad..efb0296813e9946f2b2fa1fd867592315cc4ee4f 100644 --- a/src/sbus/sssd_dbus.h +++ b/src/sbus/sssd_dbus.h @@ -246,6 +246,10 @@ int sbus_request_return_and_finish(struct sbus_request *dbus_req, int first_arg_type, ...);
+int sbus_request_return_as_variant(struct sbus_request *dbus_req,
int type,const void *value);/*
- Return an error for a DBus method call request. The @error is a normal
- DBusError.
diff --git a/src/sbus/sssd_dbus_request.c b/src/sbus/sssd_dbus_request.c index 2d23931aa07a83c15c30b31615d5db7cfee11a0a..af439b284024b2b335adc0119f131dd6046c61bc 100644 --- a/src/sbus/sssd_dbus_request.c +++ b/src/sbus/sssd_dbus_request.c @@ -125,6 +125,63 @@ int sbus_request_return_and_finish(struct sbus_request *dbus_req, return ret; }
+int sbus_request_return_as_variant(struct sbus_request *dbus_req,
int type,const void *value)+{
- DBusMessage *reply;
- dbus_bool_t dbret;
- DBusMessageIter iter;
- DBusMessageIter valiter;
- char strtype[2];
- int ret;
- snprintf(strtype, 2, "%c", type);
- reply = dbus_message_new_method_return(dbus_req->message);
- if (reply == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "Out of memory allocating DBus message\n");sbus_request_finish(dbus_req, NULL);return ENOMEM;- }
- dbus_message_iter_init_append(reply, &iter);
- dbret = dbus_message_iter_open_container(&iter, DBUS_TYPE_VARIANT,
strtype, &valiter);- if (!dbret) {
sbus_request_finish(dbus_req, NULL);ret = EINVAL;goto done;- }
- dbret = dbus_message_iter_append_basic(&valiter, type, value);
- if (!dbret) {
sbus_request_finish(dbus_req, NULL);ret = EINVAL;goto done;- }
- dbret = dbus_message_iter_close_container(&iter, &valiter);
- if (!dbret) {
sbus_request_finish(dbus_req, NULL);ret = EINVAL;goto done;- }
// dbret will be 0 after this line
- if (dbret) {
ret = sbus_request_finish(dbus_req, reply);- } else {
DEBUG(SSSDBG_CRIT_FAILURE, "Couldn't build DBus message\n");sbus_request_finish(dbus_req, NULL);ret = EINVAL;// this branch is dead code
- }
+done:
- dbus_message_unref(reply);
- return ret;
+}
int sbus_request_fail_and_finish(struct sbus_request *dbus_req, const DBusError *error) { -- 1.9.0
From a0fe9eb748301ed5df475ff55e375ba2bcd75f31 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Mon, 12 May 2014 19:00:36 +0200 Subject: [PATCH 2/6] SBUS: Utility function sbus_request_return_array_as_variant
Adds a utility function that returns an array of types values, each of a given size, with a given type in a variant. This utility function will be used by the GetAll property call.
src/sbus/sssd_dbus.h | 6 +++ src/sbus/sssd_dbus_request.c | 103 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 109 insertions(+)
diff --git a/src/sbus/sssd_dbus_request.c b/src/sbus/sssd_dbus_request.c index af439b284024b2b335adc0119f131dd6046c61bc..f2870077d6dc96b94c083ccdc9fc9a4c00c0428d 100644 --- a/src/sbus/sssd_dbus_request.c +++ b/src/sbus/sssd_dbus_request.c @@ -182,6 +182,109 @@ done: } +int sbus_request_return_array_as_variant(struct sbus_request *dbus_req,
int type,uint8_t *values,const int len,const size_t item_size)+{
// snip
- dbret = dbus_message_iter_close_container(&iter, &variant_iter);
- if (!dbret) {
sbus_request_finish(dbus_req, NULL);ret = EINVAL;goto done;- }
// dbret will be 0 after this line
- if (dbret) {
ret = sbus_request_finish(dbus_req, reply);- } else {
DEBUG(SSSDBG_CRIT_FAILURE, "Couldn't build DBus message\n");sbus_request_finish(dbus_req, NULL);ret = EINVAL;// the same problem as in 1st patch.
- }
+done:
- if (reply != NULL) {
dbus_message_unref(reply);- }
- talloc_free(tmp_ctx);
- return ret;
+}
int sbus_request_fail_and_finish(struct sbus_request *dbus_req, const DBusError *error) { -- 1.9.0
From 5733ceac904342a7a979d0efe066afd9f093ebe9 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Tue, 22 Apr 2014 21:50:28 +0200 Subject: [PATCH 3/6] IFP: Allow Set, Get and GetAll from DBus.Properties
The InfoPipe will support all three of: DBus.Properties.Get DBus.Properties.GetAll DBus.Properties.Set
Hence it must allow these calls to be received.
src/responder/ifp/org.freedesktop.sssd.infopipe.conf | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/src/responder/ifp/org.freedesktop.sssd.infopipe.conf b/src/responder/ifp/org.freedesktop.sssd.infopipe.conf index 56e460aa11c2534e95092fb10216b05c50c04d59..4f2a906ba812d46b6bca401d3fc14baa5c18de50 100644 --- a/src/responder/ifp/org.freedesktop.sssd.infopipe.conf +++ b/src/responder/ifp/org.freedesktop.sssd.infopipe.conf @@ -19,6 +19,16 @@ <allow send_destination="org.freedesktop.sssd.infopipe" send_interface="org.freedesktop.DBus.Introspectable"/>
- <allow send_destination="org.freedesktop.sssd.infopipe"
send_interface="org.freedesktop.DBus.Properties"send_member="GetAll"/>- <allow send_destination="org.freedesktop.sssd.infopipe"
send_interface="org.freedesktop.DBus.Properties"send_member="Get"/>- <allow send_destination="org.freedesktop.sssd.infopipe"
send_interface="org.freedesktop.DBus.Properties"send_member="Set"/><allow send_interface="org.freedesktop.sssd.infopipe"/> </policy>
-- 1.9.0
From d23886c484bc955bf4bc03998601aeec1650ce02 Mon Sep 17 00:00:00 2001 From: Stef Walter stefw@redhat.com Date: Tue, 25 Feb 2014 18:31:03 +0100 Subject: [PATCH 4/6] WIP properties
I knot that is is a Stef's patch, but it does not look professional to have WIP patch in master.
From 796d392b6cb33d6ded6ae58e1abcca84cf6a9b25 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Wed, 23 Apr 2014 03:01:21 +0200 Subject: [PATCH 5/6] SBUS: Implement org.freedesktop.DBus.Properties.Get for primitive types
This patch implements type-safe getters for primitive types and their arrays. The patch includes unit tests of all supported types and arrays of these types.
All getter are synchronous. The getters never fail, instead, they return a default or 'not defined' value. Making the getters synchronous and always returning a value will make it significantly easier to implement the GetAll method.
src/sbus/sbus_codegen | 114 ++++--- src/sbus/sssd_dbus.h | 6 + src/sbus/sssd_dbus_meta.h | 2 + src/sbus/sssd_dbus_properties.c | 75 +++++ src/tests/sbus_codegen_tests.c | 490 ++++++++++++++++++++++++++++- src/tests/sbus_codegen_tests.xml | 26 ++ src/tests/sbus_codegen_tests_generated.c | 522 +++++++++++++++++++++++++++++++ src/tests/sbus_codegen_tests_generated.h | 44 +++ 8 files changed, 1235 insertions(+), 44 deletions(-)
//snip
diff --git a/src/sbus/sssd_dbus_properties.c b/src/sbus/sssd_dbus_properties.c index 835b3078b5abd124ab98265401aa48533a306ee3..bdd5b432fe57ca66a4d06f98720578b3a175a65c 100644 --- a/src/sbus/sssd_dbus_properties.c +++ b/src/sbus/sssd_dbus_properties.c @@ -105,6 +105,77 @@ dispatch_properties_set(struct sbus_connection *conn, return EOK; }
+static int +dispatch_properties_get(struct sbus_connection *conn,
struct sbus_interface *intf,DBusMessage *message)+{
- struct sbus_request *req;
- const char *signature;
- const struct sbus_interface_meta *meta;
- DBusMessageIter iter;
- sbus_msg_handler_fn handler_fn;
- const struct sbus_property_meta *property;
- const char *interface_name;
- const char *property_name;
- req = sbus_new_request(conn, intf, message);
- if (req == NULL) {
return ENOMEM;- }
- meta = intf->vtable->meta;
- signature = dbus_message_get_signature(message);
- /* Interface name, property name */
- if (strcmp(signature, "ss") != 0) {
return sbus_request_fail_and_finish(req,sbus_error_new(req,DBUS_ERROR_INVALID_ARGS,"Invalid argument types passed to Get method"));- }
- dbus_message_iter_init(message, &iter);
- dbus_message_iter_get_basic(&iter, &interface_name);
- dbus_message_iter_next(&iter);
- dbus_message_iter_get_basic(&iter, &property_name);
- if (strcmp(interface_name, meta->name) != 0) {
return sbus_request_fail_and_finish(req,sbus_error_new(req,DBUS_ERROR_UNKNOWN_INTERFACE,"No such interface"));- }
- property = sbus_meta_find_property(intf->vtable->meta, property_name);
- if (property == NULL) {
return sbus_request_fail_and_finish(req,sbus_error_new(req,DBUS_ERROR_UNKNOWN_PROPERTY,"No such property"));- }
- if (!(property->flags & SBUS_PROPERTY_READABLE)) {
return sbus_request_fail_and_finish(req,sbus_error_new(req,DBUS_ERROR_ACCESS_DENIED,"Property is not readable"));- }
- handler_fn = VTABLE_FUNC(intf->vtable, property->vtable_offset_get);
- if (!handler_fn) {
return sbus_request_fail_and_finish(req,sbus_error_new(req,DBUS_ERROR_NOT_SUPPORTED,"Not implemented"));- }
- sbus_request_invoke_or_finish(req, handler_fn,
intf->instance_data,property->invoker_get);- return EOK;
+}
int sbus_properties_dispatch(struct sbus_request *dbus_req) { const char *member; @@ -116,6 +187,10 @@ int sbus_properties_dispatch(struct sbus_request *dbus_req) return dispatch_properties_set(dbus_req->conn, dbus_req->intf, dbus_req->message);
- } else if (strcmp (member, "Get") == 0) {
return dispatch_properties_get(dbus_req->conn,dbus_req->intf,dbus_req->message);The last two patches break build on rhel6.
src/sbus/sssd_dbus_properties.c:66: error: 'DBUS_ERROR_UNKNOWN_INTERFACE' undeclared (first use in this function) src/sbus/sssd_dbus_properties.c:66: error: (Each undeclared identifier is reported only once src/sbus/sssd_dbus_properties.c:66: error: for each function it appears in.) src/sbus/sssd_dbus_properties.c:74: error: 'DBUS_ERROR_UNKNOWN_PROPERTY' undeclared (first use in this function) src/sbus/sssd_dbus_properties.c:81: error: 'DBUS_ERROR_PROPERTY_READ_ONLY' undeclared (first use in this function) src/sbus/sssd_dbus_properties.c: In function 'dispatch_properties_get': src/sbus/sssd_dbus_properties.c:146: error: 'DBUS_ERROR_UNKNOWN_INTERFACE' undeclared (first use in this function) src/sbus/sssd_dbus_properties.c:154: error: 'DBUS_ERROR_UNKNOWN_PROPERTY' undeclared (first use in this function) make[2]: *** [src/sbus/libsss_util_la-sssd_dbus_properties.lo] Error 1
Yes, these errors and related constants were a late addition to DBus. When implementing a service, we should #define them when not present.
Thanks for catching this, I've added the constants in a new patch. I've only added those we use currently -- it would be easy to spot if we start using some other constant that is not present on el6 because the build would simply fail and I wouldn't like to add more legacy cruft than necessary. But if you think it's better to be safe and #define them all, I wouldn't cry too hard either.
By the way, if we ever *call* an external services, we should handle org.freedesktop.DBus.Error.UnknownMethod (ie: DBUS_ERROR_UNKNOWN_METHOD) as a stand-in for UnknownObject, UnknownInterface, UnknownProperty if the service returns it.
OK, noted.
Stef
On Tue, May 13, 2014 at 06:04:06PM +0200, Lukas Slebodnik wrote:
- dbret = dbus_message_iter_close_container(&iter, &valiter);
- if (!dbret) {
sbus_request_finish(dbus_req, NULL);ret = EINVAL;goto done;- }
// dbret will be 0 after this line
I've removed the above and only let the if-else below. Thanks for catching this.
- if (dbret) {
ret = sbus_request_finish(dbus_req, reply);- } else {
DEBUG(SSSDBG_CRIT_FAILURE, "Couldn't build DBus message\n");sbus_request_finish(dbus_req, NULL);ret = EINVAL;// this branch is dead code
- }
+done:
- dbus_message_unref(reply);
- return ret;
+}
On 12.05.2014 23:27, Jakub Hrozek wrote:
Hi,
the attached patchset implement the org.freedesktop.DBus.Properties.Get() interface for properties of primitive types and arrays of primitive types (except for array of bools as DBus bool is of different size than C-sized bool).
This looks nice to me. A few comments.
+ dbus_message_iter_init_append(reply, &iter); + dbret = dbus_message_iter_open_container(&iter, DBUS_TYPE_VARIANT, + strtype, &valiter); + if (!dbret) { + sbus_request_finish(dbus_req, NULL); + ret = EINVAL; + goto done; + }
You probably want some diagnostics here, as this is where you'll end up if you pass in an invalid @type.
+ dbus_message_iter_init_append(reply, &iter); + dbret = dbus_message_iter_open_container(&iter, DBUS_TYPE_VARIANT, + variant_type, &variant_iter); + if (!dbret) { + sbus_request_finish(dbus_req, NULL); + ret = EINVAL; + goto done; + } + + /* Open container for values */ + dbret = dbus_message_iter_open_container(&variant_iter, DBUS_TYPE_ARRAY, + array_type, &array_iter); + if (!dbret) { + sbus_request_finish(dbus_req, NULL); + ret = EINVAL; + goto done; + }
Same here and here.
- for (i = 0; i < len; i++) {
addr = values + i * item_size;dbret = dbus_message_iter_append_basic(&array_iter, type, addr);if (!dbret) {sbus_request_finish(dbus_req, NULL);ret = ENOMEM;goto done;}- }
Are you planning on using this for byte arrays and such? If so it may be worth looking into special casing arrays of bytes/ints with dbus_message_iter_append_fixed_array()
http://dbus.freedesktop.org/doc/api/html/group__DBusMessage.html#ga6e2d1e936...
But maybe that's not needed in this initial patchset?
From d23886c484bc955bf4bc03998601aeec1650ce02 Mon Sep 17 00:00:00 2001 From: Stef Walter stefw@redhat.com Date: Tue, 25 Feb 2014 18:31:03 +0100 Subject: [PATCH 4/6] WIP properties
Should I give this a better commit message? :)
- if prop.type == "s":
out(" out_val = prop_val == NULL ? \"\" : prop_val;")
You'll probably also want to special case object paths, returning "/" in those cases.
Stef
On Tue, May 13, 2014 at 07:04:22PM +0200, Stef Walter wrote:
On 12.05.2014 23:27, Jakub Hrozek wrote:
Hi,
the attached patchset implement the org.freedesktop.DBus.Properties.Get() interface for properties of primitive types and arrays of primitive types (except for array of bools as DBus bool is of different size than C-sized bool).
This looks nice to me. A few comments.
- dbus_message_iter_init_append(reply, &iter);
- dbret = dbus_message_iter_open_container(&iter, DBUS_TYPE_VARIANT,
strtype, &valiter);- if (!dbret) {
sbus_request_finish(dbus_req, NULL);ret = EINVAL;goto done;- }
You probably want some diagnostics here, as this is where you'll end up if you pass in an invalid @type.
Thanks, I was fooled by the doxygen comment that said the function only returns false on OOM. But the code is pretty conclusive, so I've added a nicer error messages for this case and the others as well.
- for (i = 0; i < len; i++) {
addr = values + i * item_size;dbret = dbus_message_iter_append_basic(&array_iter, type, addr);if (!dbret) {sbus_request_finish(dbus_req, NULL);ret = ENOMEM;goto done;}- }
Are you planning on using this for byte arrays and such? If so it may be worth looking into special casing arrays of bytes/ints with dbus_message_iter_append_fixed_array()
http://dbus.freedesktop.org/doc/api/html/group__DBusMessage.html#ga6e2d1e936...
But maybe that's not needed in this initial patchset?
I haven't changed this part in the attached patchset, but I'm fine with the change you propose -- maybe I just don't see the rationale. Is there a performance boost? Is it worth the additional if-else?
From d23886c484bc955bf4bc03998601aeec1650ce02 Mon Sep 17 00:00:00 2001 From: Stef Walter stefw@redhat.com Date: Tue, 25 Feb 2014 18:31:03 +0100 Subject: [PATCH 4/6] WIP properties
Should I give this a better commit message? :)
Oops, that's something I meant to change before sending the patches and then didn't. The attached patchset changes the message to something hopefully better..feel free to propose a different commit message, though.
- if prop.type == "s":
out(" out_val = prop_val == NULL ? \"\" : prop_val;")You'll probably also want to special case object paths, returning "/" in those cases.
Yes. I also added unit tests for both these special cases.
Thank you very much for the review. New patches are attached.
On (16/05/14 00:48), Jakub Hrozek wrote:
On Tue, May 13, 2014 at 07:04:22PM +0200, Stef Walter wrote:
On 12.05.2014 23:27, Jakub Hrozek wrote:
Hi,
the attached patchset implement the org.freedesktop.DBus.Properties.Get() interface for properties of primitive types and arrays of primitive types (except for array of bools as DBus bool is of different size than C-sized bool).
This looks nice to me. A few comments.
- dbus_message_iter_init_append(reply, &iter);
- dbret = dbus_message_iter_open_container(&iter, DBUS_TYPE_VARIANT,
strtype, &valiter);- if (!dbret) {
sbus_request_finish(dbus_req, NULL);ret = EINVAL;goto done;- }
You probably want some diagnostics here, as this is where you'll end up if you pass in an invalid @type.
Thanks, I was fooled by the doxygen comment that said the function only returns false on OOM. But the code is pretty conclusive, so I've added a nicer error messages for this case and the others as well.
- for (i = 0; i < len; i++) {
addr = values + i * item_size;dbret = dbus_message_iter_append_basic(&array_iter, type, addr);if (!dbret) {sbus_request_finish(dbus_req, NULL);ret = ENOMEM;goto done;}- }
Are you planning on using this for byte arrays and such? If so it may be worth looking into special casing arrays of bytes/ints with dbus_message_iter_append_fixed_array()
http://dbus.freedesktop.org/doc/api/html/group__DBusMessage.html#ga6e2d1e936...
But maybe that's not needed in this initial patchset?
I haven't changed this part in the attached patchset, but I'm fine with the change you propose -- maybe I just don't see the rationale. Is there a performance boost? Is it worth the additional if-else?
From d23886c484bc955bf4bc03998601aeec1650ce02 Mon Sep 17 00:00:00 2001 From: Stef Walter stefw@redhat.com Date: Tue, 25 Feb 2014 18:31:03 +0100 Subject: [PATCH 4/6] WIP properties
Should I give this a better commit message? :)
Oops, that's something I meant to change before sending the patches and then didn't. The attached patchset changes the message to something hopefully better..feel free to propose a different commit message, though.
- if prop.type == "s":
out(" out_val = prop_val == NULL ? \"\" : prop_val;")You'll probably also want to special case object paths, returning "/" in those cases.
Yes. I also added unit tests for both these special cases.
Thank you very much for the review. New patches are attached.
I let review for others.
I just want to write that problem with builing patches on rhle6 is fixed and warnings from static analysers are gone.
LS
On Fri, May 16, 2014 at 01:09:12PM +0200, Lukas Slebodnik wrote:
On (16/05/14 00:48), Jakub Hrozek wrote:
On Tue, May 13, 2014 at 07:04:22PM +0200, Stef Walter wrote:
On 12.05.2014 23:27, Jakub Hrozek wrote:
Hi,
the attached patchset implement the org.freedesktop.DBus.Properties.Get() interface for properties of primitive types and arrays of primitive types (except for array of bools as DBus bool is of different size than C-sized bool).
This looks nice to me. A few comments.
- dbus_message_iter_init_append(reply, &iter);
- dbret = dbus_message_iter_open_container(&iter, DBUS_TYPE_VARIANT,
strtype, &valiter);- if (!dbret) {
sbus_request_finish(dbus_req, NULL);ret = EINVAL;goto done;- }
You probably want some diagnostics here, as this is where you'll end up if you pass in an invalid @type.
Thanks, I was fooled by the doxygen comment that said the function only returns false on OOM. But the code is pretty conclusive, so I've added a nicer error messages for this case and the others as well.
- for (i = 0; i < len; i++) {
addr = values + i * item_size;dbret = dbus_message_iter_append_basic(&array_iter, type, addr);if (!dbret) {sbus_request_finish(dbus_req, NULL);ret = ENOMEM;goto done;}- }
Are you planning on using this for byte arrays and such? If so it may be worth looking into special casing arrays of bytes/ints with dbus_message_iter_append_fixed_array()
http://dbus.freedesktop.org/doc/api/html/group__DBusMessage.html#ga6e2d1e936...
But maybe that's not needed in this initial patchset?
I haven't changed this part in the attached patchset, but I'm fine with the change you propose -- maybe I just don't see the rationale. Is there a performance boost? Is it worth the additional if-else?
From d23886c484bc955bf4bc03998601aeec1650ce02 Mon Sep 17 00:00:00 2001 From: Stef Walter stefw@redhat.com Date: Tue, 25 Feb 2014 18:31:03 +0100 Subject: [PATCH 4/6] WIP properties
Should I give this a better commit message? :)
Oops, that's something I meant to change before sending the patches and then didn't. The attached patchset changes the message to something hopefully better..feel free to propose a different commit message, though.
- if prop.type == "s":
out(" out_val = prop_val == NULL ? \"\" : prop_val;")You'll probably also want to special case object paths, returning "/" in those cases.
Yes. I also added unit tests for both these special cases.
Thank you very much for the review. New patches are attached.
I let review for others.
I just want to write that problem with builing patches on rhle6 is fixed and warnings from static analysers are gone.
LS
Thanks for running the patches through static analysers and testing the build!
I'm attaching a new patchset that has some simplifications in the unit test warranted by the GetAll patchset and include the Get method in the introspection XML.
Any comments on the patches are welcome!
On 21.05.2014 22:49, Jakub Hrozek wrote:
On Fri, May 16, 2014 at 01:09:12PM +0200, Lukas Slebodnik wrote:
On (16/05/14 00:48), Jakub Hrozek wrote:
On Tue, May 13, 2014 at 07:04:22PM +0200, Stef Walter wrote:
On 12.05.2014 23:27, Jakub Hrozek wrote:
Hi,
the attached patchset implement the org.freedesktop.DBus.Properties.Get() interface for properties of primitive types and arrays of primitive types (except for array of bools as DBus bool is of different size than C-sized bool).
This looks nice to me. A few comments.
- dbus_message_iter_init_append(reply, &iter);
- dbret = dbus_message_iter_open_container(&iter, DBUS_TYPE_VARIANT,
strtype, &valiter);- if (!dbret) {
sbus_request_finish(dbus_req, NULL);ret = EINVAL;goto done;- }
You probably want some diagnostics here, as this is where you'll end up if you pass in an invalid @type.
Thanks, I was fooled by the doxygen comment that said the function only returns false on OOM. But the code is pretty conclusive, so I've added a nicer error messages for this case and the others as well.
- for (i = 0; i < len; i++) {
addr = values + i * item_size;dbret = dbus_message_iter_append_basic(&array_iter, type, addr);if (!dbret) {sbus_request_finish(dbus_req, NULL);ret = ENOMEM;goto done;}- }
Are you planning on using this for byte arrays and such? If so it may be worth looking into special casing arrays of bytes/ints with dbus_message_iter_append_fixed_array()
http://dbus.freedesktop.org/doc/api/html/group__DBusMessage.html#ga6e2d1e936...
But maybe that's not needed in this initial patchset?
I haven't changed this part in the attached patchset, but I'm fine with the change you propose -- maybe I just don't see the rationale. Is there a performance boost? Is it worth the additional if-else?
From d23886c484bc955bf4bc03998601aeec1650ce02 Mon Sep 17 00:00:00 2001 From: Stef Walter stefw@redhat.com Date: Tue, 25 Feb 2014 18:31:03 +0100 Subject: [PATCH 4/6] WIP properties
Should I give this a better commit message? :)
Oops, that's something I meant to change before sending the patches and then didn't. The attached patchset changes the message to something hopefully better..feel free to propose a different commit message, though.
- if prop.type == "s":
out(" out_val = prop_val == NULL ? \"\" : prop_val;")You'll probably also want to special case object paths, returning "/" in those cases.
Yes. I also added unit tests for both these special cases.
Thank you very much for the review. New patches are attached.
I let review for others.
I just want to write that problem with builing patches on rhle6 is fixed and warnings from static analysers are gone.
LS
Thanks for running the patches through static analysers and testing the build!
I'm attaching a new patchset that has some simplifications in the unit test warranted by the GetAll patchset and include the Get method in the introspection XML.
Any comments on the patches are welcome!
ACK.
Although I do wonder if all interfaces should have the properties interface in their introspect data unconditionally. I guess pretty much any infopipe interface will have DBus Properties ... so this is fine for now.
Stef
On Thu, May 22, 2014 at 01:09:06PM +0200, Stef Walter wrote:
On 21.05.2014 22:49, Jakub Hrozek wrote:
On Fri, May 16, 2014 at 01:09:12PM +0200, Lukas Slebodnik wrote:
On (16/05/14 00:48), Jakub Hrozek wrote:
On Tue, May 13, 2014 at 07:04:22PM +0200, Stef Walter wrote:
On 12.05.2014 23:27, Jakub Hrozek wrote:
Hi,
the attached patchset implement the org.freedesktop.DBus.Properties.Get() interface for properties of primitive types and arrays of primitive types (except for array of bools as DBus bool is of different size than C-sized bool).
This looks nice to me. A few comments.
- dbus_message_iter_init_append(reply, &iter);
- dbret = dbus_message_iter_open_container(&iter, DBUS_TYPE_VARIANT,
strtype, &valiter);- if (!dbret) {
sbus_request_finish(dbus_req, NULL);ret = EINVAL;goto done;- }
You probably want some diagnostics here, as this is where you'll end up if you pass in an invalid @type.
Thanks, I was fooled by the doxygen comment that said the function only returns false on OOM. But the code is pretty conclusive, so I've added a nicer error messages for this case and the others as well.
- for (i = 0; i < len; i++) {
addr = values + i * item_size;dbret = dbus_message_iter_append_basic(&array_iter, type, addr);if (!dbret) {sbus_request_finish(dbus_req, NULL);ret = ENOMEM;goto done;}- }
Are you planning on using this for byte arrays and such? If so it may be worth looking into special casing arrays of bytes/ints with dbus_message_iter_append_fixed_array()
http://dbus.freedesktop.org/doc/api/html/group__DBusMessage.html#ga6e2d1e936...
But maybe that's not needed in this initial patchset?
I haven't changed this part in the attached patchset, but I'm fine with the change you propose -- maybe I just don't see the rationale. Is there a performance boost? Is it worth the additional if-else?
From d23886c484bc955bf4bc03998601aeec1650ce02 Mon Sep 17 00:00:00 2001 From: Stef Walter stefw@redhat.com Date: Tue, 25 Feb 2014 18:31:03 +0100 Subject: [PATCH 4/6] WIP properties
Should I give this a better commit message? :)
Oops, that's something I meant to change before sending the patches and then didn't. The attached patchset changes the message to something hopefully better..feel free to propose a different commit message, though.
- if prop.type == "s":
out(" out_val = prop_val == NULL ? \"\" : prop_val;")You'll probably also want to special case object paths, returning "/" in those cases.
Yes. I also added unit tests for both these special cases.
Thank you very much for the review. New patches are attached.
I let review for others.
I just want to write that problem with builing patches on rhle6 is fixed and warnings from static analysers are gone.
LS
Thanks for running the patches through static analysers and testing the build!
I'm attaching a new patchset that has some simplifications in the unit test warranted by the GetAll patchset and include the Get method in the introspection XML.
Any comments on the patches are welcome!
ACK.
Although I do wonder if all interfaces should have the properties interface in their introspect data unconditionally. I guess pretty much any infopipe interface will have DBus Properties ... so this is fine for now.
Stef
I was wondering that as well and wasn't sure what's the best practice in the DBus world. I can only add the interface to Introspection if there are properties in the meta, if that's what you'd prefer.
My reasoning was that you can always send the Get method to the interface, even if there are no properties, the sbus handler would just kick you out.
On 22.05.2014 14:00, Jakub Hrozek wrote:
On Thu, May 22, 2014 at 01:09:06PM +0200, Stef Walter wrote:
On 21.05.2014 22:49, Jakub Hrozek wrote:
On Fri, May 16, 2014 at 01:09:12PM +0200, Lukas Slebodnik wrote:
On (16/05/14 00:48), Jakub Hrozek wrote:
On Tue, May 13, 2014 at 07:04:22PM +0200, Stef Walter wrote:
On 12.05.2014 23:27, Jakub Hrozek wrote: > Hi, > > the attached patchset implement the > org.freedesktop.DBus.Properties.Get() interface for properties of > primitive types and arrays of primitive types (except for array of bools > as DBus bool is of different size than C-sized bool).
This looks nice to me. A few comments.
- dbus_message_iter_init_append(reply, &iter);
- dbret = dbus_message_iter_open_container(&iter, DBUS_TYPE_VARIANT,
strtype, &valiter);- if (!dbret) {
sbus_request_finish(dbus_req, NULL);ret = EINVAL;goto done;- }
You probably want some diagnostics here, as this is where you'll end up if you pass in an invalid @type.
Thanks, I was fooled by the doxygen comment that said the function only returns false on OOM. But the code is pretty conclusive, so I've added a nicer error messages for this case and the others as well.
> + for (i = 0; i < len; i++) { > + addr = values + i * item_size; > + dbret = dbus_message_iter_append_basic(&array_iter, type, addr); > + if (!dbret) { > + sbus_request_finish(dbus_req, NULL); > + ret = ENOMEM; > + goto done; > + } > + }
Are you planning on using this for byte arrays and such? If so it may be worth looking into special casing arrays of bytes/ints with dbus_message_iter_append_fixed_array()
http://dbus.freedesktop.org/doc/api/html/group__DBusMessage.html#ga6e2d1e936...
But maybe that's not needed in this initial patchset?
I haven't changed this part in the attached patchset, but I'm fine with the change you propose -- maybe I just don't see the rationale. Is there a performance boost? Is it worth the additional if-else?
> From d23886c484bc955bf4bc03998601aeec1650ce02 Mon Sep 17 00:00:00 2001 > From: Stef Walter stefw@redhat.com > Date: Tue, 25 Feb 2014 18:31:03 +0100 > Subject: [PATCH 4/6] WIP properties
Should I give this a better commit message? :)
Oops, that's something I meant to change before sending the patches and then didn't. The attached patchset changes the message to something hopefully better..feel free to propose a different commit message, though.
> + if prop.type == "s": > + out(" out_val = prop_val == NULL ? "" : prop_val;")
You'll probably also want to special case object paths, returning "/" in those cases.
Yes. I also added unit tests for both these special cases.
Thank you very much for the review. New patches are attached.
I let review for others.
I just want to write that problem with builing patches on rhle6 is fixed and warnings from static analysers are gone.
LS
Thanks for running the patches through static analysers and testing the build!
I'm attaching a new patchset that has some simplifications in the unit test warranted by the GetAll patchset and include the Get method in the introspection XML.
Any comments on the patches are welcome!
ACK.
Although I do wonder if all interfaces should have the properties interface in their introspect data unconditionally. I guess pretty much any infopipe interface will have DBus Properties ... so this is fine for now.
Stef
I was wondering that as well and wasn't sure what's the best practice in the DBus world. I can only add the interface to Introspection if there are properties in the meta, if that's what you'd prefer.
My reasoning was that you can always send the Get method to the interface, even if there are no properties, the sbus handler would just kick you out.
Makes sense ... and that's likely how it's implemented in a lot of places. Haven't checked.
Stef
On Thu, May 22, 2014 at 03:36:06PM +0200, Stef Walter wrote:
On 22.05.2014 14:00, Jakub Hrozek wrote:
On Thu, May 22, 2014 at 01:09:06PM +0200, Stef Walter wrote:
On 21.05.2014 22:49, Jakub Hrozek wrote:
On Fri, May 16, 2014 at 01:09:12PM +0200, Lukas Slebodnik wrote:
On (16/05/14 00:48), Jakub Hrozek wrote:
On Tue, May 13, 2014 at 07:04:22PM +0200, Stef Walter wrote: > On 12.05.2014 23:27, Jakub Hrozek wrote: >> Hi, >> >> the attached patchset implement the >> org.freedesktop.DBus.Properties.Get() interface for properties of >> primitive types and arrays of primitive types (except for array of bools >> as DBus bool is of different size than C-sized bool). > > This looks nice to me. A few comments. > > + dbus_message_iter_init_append(reply, &iter); > + dbret = dbus_message_iter_open_container(&iter, DBUS_TYPE_VARIANT, > + strtype, &valiter); > + if (!dbret) { > + sbus_request_finish(dbus_req, NULL); > + ret = EINVAL; > + goto done; > + } > > You probably want some diagnostics here, as this is where you'll end up > if you pass in an invalid @type.
Thanks, I was fooled by the doxygen comment that said the function only returns false on OOM. But the code is pretty conclusive, so I've added a nicer error messages for this case and the others as well.
> >> + for (i = 0; i < len; i++) { >> + addr = values + i * item_size; >> + dbret = dbus_message_iter_append_basic(&array_iter, type, addr); >> + if (!dbret) { >> + sbus_request_finish(dbus_req, NULL); >> + ret = ENOMEM; >> + goto done; >> + } >> + } > > Are you planning on using this for byte arrays and such? If so it may be > worth looking into special casing arrays of bytes/ints with > dbus_message_iter_append_fixed_array() > > http://dbus.freedesktop.org/doc/api/html/group__DBusMessage.html#ga6e2d1e936... > > But maybe that's not needed in this initial patchset?
I haven't changed this part in the attached patchset, but I'm fine with the change you propose -- maybe I just don't see the rationale. Is there a performance boost? Is it worth the additional if-else?
> >> From d23886c484bc955bf4bc03998601aeec1650ce02 Mon Sep 17 00:00:00 2001 >> From: Stef Walter stefw@redhat.com >> Date: Tue, 25 Feb 2014 18:31:03 +0100 >> Subject: [PATCH 4/6] WIP properties > > Should I give this a better commit message? :)
Oops, that's something I meant to change before sending the patches and then didn't. The attached patchset changes the message to something hopefully better..feel free to propose a different commit message, though.
> >> + if prop.type == "s": >> + out(" out_val = prop_val == NULL ? "" : prop_val;") > > You'll probably also want to special case object paths, returning "/" in > those cases.
Yes. I also added unit tests for both these special cases.
Thank you very much for the review. New patches are attached.
I let review for others.
I just want to write that problem with builing patches on rhle6 is fixed and warnings from static analysers are gone.
LS
Thanks for running the patches through static analysers and testing the build!
I'm attaching a new patchset that has some simplifications in the unit test warranted by the GetAll patchset and include the Get method in the introspection XML.
Any comments on the patches are welcome!
ACK.
Although I do wonder if all interfaces should have the properties interface in their introspect data unconditionally. I guess pretty much any infopipe interface will have DBus Properties ... so this is fine for now.
Stef
I was wondering that as well and wasn't sure what's the best practice in the DBus world. I can only add the interface to Introspection if there are properties in the meta, if that's what you'd prefer.
My reasoning was that you can always send the Get method to the interface, even if there are no properties, the sbus handler would just kick you out.
Makes sense ... and that's likely how it's implemented in a lot of places. Haven't checked.
Stef
OK, I will keep the patches as they are for now.
For what it's worth, I checked systemd when I implemented the patch as systemd is one of the few projects that implement a DBus interface without using glib.
Thanks a lot for the review, Stef, much appreciated!
On Thu, May 22, 2014 at 04:01:31PM +0200, Jakub Hrozek wrote:
ACK.
Although I do wonder if all interfaces should have the properties interface in their introspect data unconditionally. I guess pretty much any infopipe interface will have DBus Properties ... so this is fine for now.
Stef
I was wondering that as well and wasn't sure what's the best practice in the DBus world. I can only add the interface to Introspection if there are properties in the meta, if that's what you'd prefer.
My reasoning was that you can always send the Get method to the interface, even if there are no properties, the sbus handler would just kick you out.
Makes sense ... and that's likely how it's implemented in a lot of places. Haven't checked.
Stef
OK, I will keep the patches as they are for now.
For what it's worth, I checked systemd when I implemented the patch as systemd is one of the few projects that implement a DBus interface without using glib.
Thanks a lot for the review, Stef, much appreciated!
Pushed all to master: 0d8fbc2cb822b4fef90588ca368ceb883b8379f2 c5a592920fab217fdb3aaa897e68f5643de88dc4 5de968e80ade1c02d1907834dcff95e9fc9ad10a 58229439447d5617913a5a2e173b78105c694842 90e04eae7e54ec892a6f239783df94dab5d1ed9a 1319e71fd1680ca4864afe0b1aca2b8c8e4a1ee4 e412c809508be9cdd30da679b92ed9f7292357e9 f2ea63e48812b042d36ac8357f0cb13b2f2184fe e871517a0bfb806cb2fddc76ff2f59360437a73c
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Lukas Slebodnik -
Pavel Březina -
Stef Walter