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

Jakub Hrozek jhrozek at redhat.com
Fri Mar 14 12:41:08 UTC 2014


On Wed, Mar 12, 2014 at 12:01:58PM +0100, Lukas Slebodnik wrote:
> 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

And ACK++ by me as well. I really like the overall design, especially
the struct sbus_request.



More information about the sssd-devel mailing list