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

Lukas Slebodnik lslebodn at redhat.com
Mon Feb 24 15:23:47 UTC 2014


On (24/02/14 11:42), Stef Walter wrote:
>Here's the next patchset for refactoring the DBus support in sssd.
>
>This patch set reorganizes how handlers handle requests. At center stage
>is 'struct sbus_request' which is a talloc context valid for the
>duration of the DBus requests.
>
>There are also various sbus_request_xxx_finish() methods which reply to
>the caller and cleanup the request.
>
>The next set of patches (after this one) have the support for
>automatically invoking type-safe handlers and build off of this
>patchset. That said, even on this own, these changes result in lots of
>cleanup and some code savings.
>
>Patch 0001 fixes portability bugs.
>
>I've added some test cases for the sbus code that has changed.
>
>However since there are no automatic tests for much of the monitor and
>data provider methods, I would recommend lots of smoke testing for these
>patches.
>
>If you prefer to access this as a branch, see:
>
>https://github.com/stefwalter/sssd/tree/dbus-request
>
>To see the later work that builds off of this:
>
>https://github.com/stefwalter/sssd/tree/dbus-invoke
>
>Cheers,
>
>Stef

I didn't test patches. I am only sending warnings from compiler and
static analysers.

>From 2cd571b34e2346b7ea221c3ec76afd0db3a6c33b 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        | 516 +++++++++++---------------------
> 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                    |  79 ++++-
> src/sbus/sssd_dbus_connection.c         |  43 +--
> src/sbus/sssd_dbus_private.h            |   6 +
> src/sbus/sssd_dbus_request.c            | 111 +++++++
> src/tests/sbus_codegen_tests.c          |   4 +-
> 16 files changed, 456 insertions(+), 512 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..af33027 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;
>+    struct sbus_request *req = NULL;
>     sbus_msg_handler_fn handler_fn;
>+    DBusHandlerResult result;
>     int ret;
> 
>     if (!user_data) {
>@@ -436,6 +438,8 @@ 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) {
>@@ -451,6 +455,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 +464,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 +474,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) {
          ^^^^^^^^^^
There is an warning: variable 'handler_fn' is used uninitialized
This can happen if  strcmp(msg_interface, interface->name) != 0 (line 405)

>+        req = sbus_new_request(intf_p->conn, intf_p->intf, message);
>+        if (!req) {
>+            ret = ENOMEM;
>+        } else {
>+            req->method = method;
>+            ret = handler_fn(req);
>+        }
>+        if (ret != EOK) {
>+            if (req)
>+                talloc_free(req);
>+            result = sbus_reply_internal_error(message, intf_p->conn);
>+        } else {
>+            result = DBUS_HANDLER_RESULT_HANDLED;
>         }
>-        else
>-            return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
>     }
> 
>-    return DBUS_HANDLER_RESULT_HANDLED;
>+    return result;
> }
> 
> /* Adds a new D-BUS path message handler to the connection
>@@ -786,4 +794,3 @@ void sbus_conn_send_reply(struct sbus_connection *conn, DBusMessage *reply)
> {
>     dbus_connection_send(conn->dbus.conn, reply, NULL);
> }
>-
>diff --git a/src/sbus/sssd_dbus_request.c b/src/sbus/sssd_dbus_request.c
>new file mode 100644
>index 0000000..4e78f87
>--- /dev/null
>+++ b/src/sbus/sssd_dbus_request.c
>@@ -0,0 +1,111 @@
>+/*
>+    Authors:
>+        Stef Walter <stefw at redhat.com>
>+
>+    Copyright (C) 2014 Red Hat
>+
>+    This program is free software; you can redistribute it and/or modify
>+    it under the terms of the GNU General Public License as published by
>+    the Free Software Foundation; either version 3 of the License, or
>+    (at your option) any later version.
>+
>+    This program is distributed in the hope that it will be useful,
>+    but WITHOUT ANY WARRANTY; without even the implied warranty of
>+    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>+    GNU General Public License for more details.
>+
>+    You should have received a copy of the GNU General Public License
>+    along with this program.  If not, see <http://www.gnu.org/licenses/>.
>+*/
>+
>+#include "util/util.h"
>+#include "sbus/sssd_dbus.h"
>+
>+#include <sys/time.h>
>+#include <dbus/dbus.h>
>+
>+static int sbus_request_destructor(void *data)
>+{
>+    struct sbus_request *req = data;
>+    dbus_message_unref(req->message);
>+    return 0;
>+}
>+
>+struct sbus_request *
>+sbus_new_request(struct sbus_connection *conn,
>+                 struct sbus_interface *intf,
>+                 DBusMessage *message)
>+{
>+    struct sbus_request *req;
>+
>+    req = talloc_zero(conn, struct sbus_request);
>+    if (!req)
>+        return NULL;
>+
>+    req->intf = intf;
>+    req->conn = conn;
>+    req->message = dbus_message_ref(message);
>+    talloc_set_destructor(req, sbus_request_destructor);
src/sbus/sssd_dbus_request.c:48:5: error: incompatible pointer types initializing
      'int (*)(typeof (req))' with an expression of type 'int (void *)'
      [-Werror,-Wincompatible-pointer-types]
    talloc_set_destructor(req, sbus_request_destructor);
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/talloc.h:345:9: note: expanded from macro 'talloc_set_destructor'
                int (*_talloc_destructor_fn)(_TALLOC_TYPEOF(ptr)) = (function);       \
                      ^                                             ~~~~~~~~~~
You should change prototype of sbus_request_destructor:
-static int sbus_request_destructor(void *data)
+static int sbus_request_destructor(struct sbus_request *req)


>+
>+    return req;
>+}
>+
>+int sbus_request_finish(struct sbus_request *request,
>+                        DBusMessage *reply)
>+{
>+    if (reply)
>+        sbus_conn_send_reply(request->conn, reply);
>+    return talloc_free(request);
>+}
>+
>+int sbus_request_return_and_finish(struct sbus_request *request,
>+                                   int first_arg_type,
>+                                   ...)
>+{
>+    DBusMessage *reply;
>+    dbus_bool_t ret;
>+    va_list va;
>+    int res;
>+
>+    reply = dbus_message_new_method_return(request->message);
                                             ^^^^^^^
request can be NULL

src/providers/data_provider_be.c:1640: assign_zero: Assigning: "dbus_req" = "NULL".
src/providers/data_provider_be.c:1726: var_deref_model: Passing null pointer "dbus_req" to function "sbus_request_return_and_finish(struct sbus_request *, int, ...)", which dereferences it.
src/sbus/sssd_dbus_request.c:70:5: deref_parm: Directly dereferencing parameter "request"

and the 2nd similar warning:

src/providers/data_provider_be.c:1825: assign_zero: Assigning: "dbus_req" = "NULL".
src/providers/data_provider_be.c:1903: var_deref_model: Passing null pointer "dbus_req" to function "sbus_request_return_and_finish(struct sbus_request *, int, ...)", which dereferences it.
src/sbus/sssd_dbus_request.c:70:5: deref_parm: Directly dereferencing parameter "request".
>+    if (!reply) {
>+        DEBUG(SSSDBG_CRIT_FAILURE, "Out of memory allocating DBus message\n");
>+        sbus_request_finish(request, NULL);
>+        return ENOMEM;
>+    }
>+
>+    va_start(va, first_arg_type);
>+    ret = dbus_message_append_args_valist(reply, first_arg_type, va);
>+    va_end(va);
>+
>+    if (ret) {
>+        res = sbus_request_finish(request, reply);
>+
>+    } else {
>+        DEBUG(SSSDBG_CRIT_FAILURE, "Couldn't build DBus message\n");
>+        sbus_request_finish(request, NULL);
>+        res = EINVAL;
>+    }
>+
>+    dbus_message_unref(reply);
>+    return res;
>+}

<snip>

>From 46ab7fad730dd2751e5a34ce8fd0f5137fb10bac Mon Sep 17 00:00:00 2001
>From: Stef Walter <stefw at redhat.com>
>Date: Tue, 18 Feb 2014 13:27:20 +0100
>Subject: [PATCH 5/6] sbus_tests: Add some testing of dispatch and handler code
>
>This starts a DBus server with some handlers, and runs some method
>calls against it.
>
>Note that we don't use the codegen in the sbus_tests, as we sorta
>want to test this non-codegen related functionality on its own before
>we run the sbus_codegen_tests.
>---
> Makefile.am             |  11 +++
> src/tests/common.h      |  15 ++++
> src/tests/common_dbus.c | 197 +++++++++++++++++++++++++++++++++++++++++
> src/tests/sbus_tests.c  | 230 ++++++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 453 insertions(+)
> create mode 100644 src/tests/common_dbus.c
> create mode 100644 src/tests/sbus_tests.c
>

<snip>

>diff --git a/src/tests/common_dbus.c b/src/tests/common_dbus.c
>new file mode 100644
>index 0000000..92d18d0
>--- /dev/null
>+++ b/src/tests/common_dbus.c

<snip>

>+
>+struct DBusConnection *
>+test_dbus_setup_mock(TALLOC_CTX *mem_ctx,
>+                     struct tevent_context *loop,
>+                     sbus_server_conn_init_fn init_fn,
>+                     void *init_pvt_data)
>+{
>+    struct mock_server *mock;
>+    char dummy;
>+
>+    mock = talloc_zero(mem_ctx, struct mock_server);
>+    talloc_set_destructor(mock, mock_server_cleanup);
                                  ^^^^^^^^^^^^^^^^^^^
src/tests/common_dbus.c:143:5: error: incompatible pointer types initializing
      'int (*)(typeof (mock))' with an expression of type 'int (void *)'
      [-Werror,-Wincompatible-pointer-types]
    talloc_set_destructor(mock, mock_server_cleanup);
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/talloc.h:345:9: note: expanded from macro 'talloc_set_destructor'
                int (*_talloc_destructor_fn)(_TALLOC_TYPEOF(ptr)) = (function);

You should change protype of mock_server_cleanup:
-mock_server_cleanup(void *data)
+mock_server_cleanup(struct mock_server *mock)


>+    mock->init_fn = init_fn;
>+    mock->init_pvt_data = init_pvt_data;
>+

LS



More information about the sssd-devel mailing list