[SSSD] [PATCH] Implement type-safe getters for primitive types and their arrays
Stef Walter
stefw at redhat.com
Tue May 13 18:42:19 UTC 2014
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 at 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 at 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 at 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 at 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 at 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
More information about the sssd-devel
mailing list