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

Lukas Slebodnik lslebodn at redhat.com
Wed Mar 12 11:01:58 UTC 2014


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
>

I tested sssd with valgrind (each responder and ipa provider)
    valgrind -v --leak-check=full --show-reachable=yes

getent {passwd, group, netgroup}, id --works
pam authentication  --works
sudo    --works
autofs  --works
ssh     --works

Valgrind didn't report any problem.

ACK++

LS



More information about the sssd-devel mailing list