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