[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