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(a)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(a)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(a)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(a)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(a)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