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

Pavel Březina pbrezina at redhat.com
Fri Mar 7 13:55:07 UTC 2014


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.

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.

- we usually use 'ret' instead of 'res' to indicate variable containing 
return value. And lowercased FALSE/TRUE. But that's no biggie.

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

Patch 3:
- prototype of dp handlers contains 'data', declarations have 'user_data'
- coding style issues, see #2

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.




More information about the sssd-devel mailing list