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

Stef Walter stefw at redhat.com
Mon Mar 10 08:12:23 UTC 2014


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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-providers-Fix-types-passed-to-dbus-varargs-functions.patch
Type: text/x-patch
Size: 7086 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140310/7d89eb8f/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-sbus-Add-struct-sbus_request-to-represent-a-DBus-inv.patch
Type: text/x-patch
Size: 74878 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140310/7d89eb8f/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-sbus-Refactor-how-we-export-DBus-interfaces.patch
Type: text/x-patch
Size: 56798 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140310/7d89eb8f/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-sbus-Make-sbus_new_server-work-for-non-priveleged-pr.patch
Type: text/x-patch
Size: 1075 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140310/7d89eb8f/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-sbus_tests-Add-some-testing-of-dispatch-and-handler-.patch
Type: text/x-patch
Size: 15602 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140310/7d89eb8f/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0006-sbus-Add-the-sbus_request_parse_or_finish-method.patch
Type: text/x-patch
Size: 21089 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140310/7d89eb8f/attachment-0005.bin>


More information about the sssd-devel mailing list