[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