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