[SSSD] [PATCH] DBus: Refactor how requests are handled

Lukas Slebodnik lslebodn at redhat.com
Wed Apr 16 12:51:36 UTC 2014


On (10/03/14 09:12), Stef Walter wrote:
>On 07.03.2014 14:55, Pavel Březina wrote:
>> On 02/28/2014 06:15 PM, Stef Walter wrote:
>>> On 24.02.2014 18:50, Stef Walter wrote:
>>>> On 24.02.2014 16:23, Lukas Slebodnik wrote:
>>>>> On (24/02/14 11:42), Stef Walter wrote:
>>>>>> Here's the next patchset for refactoring the DBus support in sssd.
>>>>>>
>>>>> I didn't test patches. I am only sending warnings from compiler and
>>>>> static analysers.
>>>>
>>>> Thanks. Attached are the fixed patches.
>>>
>>> Rebased on master, and tweaked sbus_request_fail_and_finish() to better
>>> accomodate code in the dbus-invoke branch.
>>>
>>> Stef
>> 
>> Hi,
>> really great job! I have several comments, mostly coding style issues.
>
>Thanks for looking it over.
>
>> Patch 1: Ack.
>> 
>> Patch 2:
>> - can you use either dbus_req or request as sbus_request name? For
>> example dp handlers have 'request' in prototype, but 'dbus_req' in
>> declaration.
>
>Sure. The main reason I did the dp stuff as 'request' was there were so
>many requests floating around that it needed differentiation. But I can
>do that everywhere if it's needed: dbus_req it is.
>
>> - we usually use 'ret' instead of 'res' to indicate variable containing
>> return value. And lowercased FALSE/TRUE. But that's no biggie.
>
>K. done.
>
>> - we use always brackets around if, if...else statements. Even for one
>> liners. Especially these types are hard to read.
>> +    if (!intf)
>> +        ret = ENOMEM;
>> +    else
>> +        ret = sbus_conn_add_interface(ctx->conn, intf);
>> +    if (ret != EOK) {
>> +        DEBUG(SSSDBG_FATAL_FAILURE, "Failed to export proxy.\n");
>> +        return ret;
>> +    }
>> 
>> +    if (!sbus_request_parse_or_finish(dbus_req,
>> +                                      DBUS_TYPE_UINT32, &type,
>> +                                      DBUS_TYPE_UINT32, &attr_type,
>> +                                      DBUS_TYPE_STRING, &filter,
>> +                                      DBUS_TYPE_STRING, &domain,
>> +                                      DBUS_TYPE_INVALID))
>> +        return EOK; /* handled */
>> 
>> 
>> Those coding style issues are valid also for the third patch.
>
>
>Alright.
>
>> Patch 3:
>> - prototype of dp handlers contains 'data', declarations have 'user_data'
>> - coding style issues, see #2
>
>Will make these consistent. I used the same argument names as the
>previous stack variables, where present.
>
>> Patch 4: Ack.
>> 
>> Patch 5:
>> - coding style issues, see #2
>> 
>> Acks are code-wise only so far. It would be nice if someone else
>> reviewed it as well, since it is large patch set and most of the changes
>> are pattern replacement, it is highly possible I have missed something.
>
>Attached is a rebased patchset with the above issues fixed. Making these
>fixes was fairly tedious, with lots of conflicts, so I hope I didn't
>miss something.
>
>Stef
>

>From 0cdba3f82dfe955842f67d48c407e204729724e8 Mon Sep 17 00:00:00 2001
>From: Stef Walter <stefw at gnome.org>
>Date: Fri, 17 Jan 2014 12:54:42 +0100
>Subject: [PATCH 2/6] sbus: Add struct sbus_request to represent a DBus
> invocation
>
>struct sbus_request represents a request from a dbus client
>being handled by a dbus server implementation. The struct
>contains the message, connection and method (and in the
>future teh property) which is being requested.
>
>In the future it will contain caller information as well.
>
>sbus_request is a talloc memory context, and is a good place to
>attach any allocations and memory specific to the request.
>
>Each handler accepts an sbus_request. If a handler returns
>EOK, it is assumed that the handler will finish the request.
>Any of the sbus_request_*finish() methods can be used to
>complete the request and send back a reply.
>
>sbus_request_return_and_finish() uses the same argument
>varargs syntax as dbus_message_append_args(), which isn't
>a great syntax. Document it a bit, but don't try to redesign:
>The marshalling work (will follow this patch set) will remove
>the need to use varargs for most DBus implementation code.
>
>This patch migrates the monitor and data provider dbus code
>to use sbus_request, but does not try to rework the talloc
>context's to use it.
>---
> Makefile.am                             |   1 +
> src/monitor/monitor.c                   |  54 +---
> src/monitor/monitor_interfaces.h        |   6 +-
> src/monitor/monitor_sbus.c              |  26 +-
> src/providers/data_provider_be.c        | 512 ++++++++++++--------------------
> src/providers/proxy/proxy_child.c       |  14 +-
> src/providers/proxy/proxy_init.c        |  38 +--
> src/responder/autofs/autofssrv.c        |  10 +-
> src/responder/common/responder.h        |   3 +-
> src/responder/common/responder_common.c |   7 +-
> src/responder/nss/nsssrv.c              |  50 +---
> src/sbus/sssd_dbus.h                    |  78 ++++-
> src/sbus/sssd_dbus_connection.c         |  46 +--
> src/sbus/sssd_dbus_private.h            |   6 +
> src/sbus/sssd_dbus_request.c            | 112 +++++++
> src/tests/sbus_codegen_tests.c          |   4 +-
> 16 files changed, 469 insertions(+), 498 deletions(-)
> create mode 100644 src/sbus/sssd_dbus_request.c

//snip

>diff --git a/src/sbus/sssd_dbus_connection.c b/src/sbus/sssd_dbus_connection.c
>index d39f1c0..5faf249 100644
>--- a/src/sbus/sssd_dbus_connection.c
>+++ b/src/sbus/sssd_dbus_connection.c
>@@ -416,7 +416,9 @@ DBusHandlerResult sbus_message_handler(DBusConnection *dbus_conn,
>     DBusMessage *reply = NULL;
>     const struct sbus_method_meta *method;
>     const struct sbus_interface_meta *interface;
>-    sbus_msg_handler_fn handler_fn;
>+    struct sbus_request *dbus_req = NULL;
>+    sbus_msg_handler_fn handler_fn = NULL;
>+    DBusHandlerResult result;
>     int ret;
> 
>     if (!user_data) {
>@@ -436,10 +438,11 @@ DBusHandlerResult sbus_message_handler(DBusConnection *dbus_conn,
>     if (strcmp(path, intf_p->intf->path) != 0)
>         return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
> 
>+    result = DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
>+
>     /* Validate the method interface */
>     interface = intf_p->intf->vtable->meta;
>     if (strcmp(msg_interface, interface->name) == 0) {
>-        handler_fn = NULL;
>         method = sbus_meta_find_method(interface, msg_method);
>         if (method && method->vtable_offset)
>             handler_fn = VTABLE_FUNC(intf_p->intf->vtable, method->vtable_offset);
>@@ -451,6 +454,7 @@ DBusHandlerResult sbus_message_handler(DBusConnection *dbus_conn,
>             reply = dbus_message_new_error(message, DBUS_ERROR_UNKNOWN_METHOD, NULL);
>             sbus_conn_send_reply(intf_p->conn, reply);
>             dbus_message_unref(reply);
>+            result = DBUS_HANDLER_RESULT_HANDLED;
> 
>         } else if (!handler_fn) {
>             /* Reply DBUS_ERROR_NOT_SUPPORTED */
>@@ -459,11 +463,7 @@ DBusHandlerResult sbus_message_handler(DBusConnection *dbus_conn,
>             reply = dbus_message_new_error(message, DBUS_ERROR_NOT_SUPPORTED, NULL);
>             sbus_conn_send_reply(intf_p->conn, reply);
>             dbus_message_unref(reply);
>-
>-        } else {
>-            ret = handler_fn(message, intf_p->conn);
>-            if (ret != EOK)
>-                return sbus_reply_internal_error(message, intf_p->conn);
>+            result = DBUS_HANDLER_RESULT_HANDLED;
>         }
>     }
>     else {
>@@ -473,21 +473,28 @@ DBusHandlerResult sbus_message_handler(DBusConnection *dbus_conn,
>         if (strcmp(msg_interface, DBUS_INTROSPECT_INTERFACE) == 0 &&
>             strcmp(msg_method, DBUS_INTROSPECT_METHOD) == 0)
>         {
>-            if (intf_p->intf->introspect_fn) {
>-                /* If we have been asked for introspection data and we have
>-                 * an introspection function registered, user that.
>-                 */
>-                ret = intf_p->intf->introspect_fn(message, intf_p->conn);
>-                if (ret != EOK) {
>-                    return sbus_reply_internal_error(message, intf_p->conn);
>-                }
>-            }
>+            handler_fn = intf_p->intf->introspect_fn;
>+        }
>+    }
>+
>+    if (handler_fn) {
>+        dbus_req = sbus_new_request(intf_p->conn, intf_p->intf, message);
>+        if (!dbus_req) {
>+            ret = ENOMEM;
>+        } else {
>+            dbus_req->method = method;
//I can see warning on fedora rawhide with default CFLAGS (-g -O2)

src/sbus/sssd_dbus_connection.c: In function 'sbus_message_handler':
src/sbus/sssd_dbus_connection.c:454:30: warning: 'method' may be used uninitialized in this function [-Wmaybe-uninitialized]
             dbus_req->method = method;

                              ^

>+            ret = handler_fn(dbus_req);
>+        }
>+        if (ret != EOK) {
>+            if (dbus_req)
>+                talloc_free(dbus_req);
>+            result = sbus_reply_internal_error(message, intf_p->conn);
>+        } else {
>+            result = DBUS_HANDLER_RESULT_HANDLED;
>         }
>-        else
>-            return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
>     }

LS



More information about the sssd-devel mailing list