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

Jakub Hrozek jhrozek at redhat.com
Fri Mar 7 16:04:07 UTC 2014


On Fri, Mar 07, 2014 at 02:55:07PM +0100, 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.
> 
> Patch 1: Ack.

btw I would like to push this patch to sssd-1-11 as well. See commit
46c5de as an example of a very similar problem that caused real world
problems for users of exotic architectures (ppc I think)

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

In Stef's defense, the sbus code used conditions w/o brackets as the
code predates our formalized coding style.

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

I'm rebasing my branch atop these patches. I figured it might be a good
way to review them.

Thanks for the review, Pavel!



More information about the sssd-devel mailing list