[SSSD] [PATCH] DP: Refactor responder_dp_req so it's reusable by other responders

Pavel Březina pbrezina at redhat.com
Mon Jan 23 15:54:10 UTC 2012


Dne 23.1.2012 11:29, Jakub Hrozek napsal(a):
> On Sun, Jan 22, 2012 at 08:14:21PM -0500, Stephen Gallagher wrote:
>> On Sun, 2012-01-22 at 22:08 +0100, Jakub Hrozek wrote:
>>> A number of new responders is currently being developed in addition to
>>> the existing NSS and PAM responders that only deal with accounts. The
>>> new responders often deal with different data such as automounter maps
>>> or sudoer rules. It is prudent to let these responders communicate with
>>> back ends using their own protocol.
>>>
>>> Attached is a patch that refactors the requests in Data Provider so that
>>> they are more generic. The commits message is slightly more verbose than
>>> usual this time to explain all the changes.
>>>
>>> I'm going to include a sudo request shortly as an example of a new
>>> non-account request.
>>>
>>> [PATCH] DP: Refactor responder_dp_req so it's reusable by other responders:
>>> * the internal requst is now more generic and is decoupled from
>>>    account-specific data. There is a new sss_dp_issue_request() wrapper
>>>    that issues a BE request or registers a callback
>>> * the public requests all use struct sss_dp_req_state as the tevent_req
>>>    state data. This allows to report back data from the internal request
>>>    even if the caller is just a callback notifier
>>> * each specific request now uses an _info structure that contains all
>>>    the data necessary to construct a DBusMessage passed to provider
>>> * each specific request now defines a sss_dp_get_$data_msg callback that
>>>    is called from the sss_dp_issue_request() common wraper. The purpose
>>>    of the wrapper is to construct a DBusMessage and bind it to a DBus
>>>    method so the message can be just sent over to back end
>>>
>>> The miscellanous changes include:
>>> * change SSS_DP_ constants to an enum. This way, a switch() would error
>>>    if a value is not handled.
>>> * rename sss_dp_get_account_int_send() to sss_dp_internal_get_send()
>>>    request because the internal request is going to handle more than just
>>>    account data
>>>
>>> The patch must be applied on top of all Stephen's patches that are
>>> currently on the list, including the services support.
>>
>> Nack.
>>
>> Just to remain consistent with the old approach, please start the enum
>> explicitly at 1.
>>
>
> Done.

Nack.

If you want the compiler to check that all enum are handled in switch() 
you have to provide a properly typed variable.

info->type has to be of type 'enum sss_dp_type'

And we have to split this enum to two - one for accounts and one for sudo.

>
>> In sss_dp_get_account_send(), you set 'ret = ENOMEM' if the key isn't
>> allocated, but you don't 'goto error'. (On further review, it looks like
>> that bug existed previously. Please fix it anyway).
>>
>
> Done.
>
>> There's no purpose to the existence of 'tvcb' in sss_dp_issue_request().
>> There should never be a case where we'd do other than just free the
>> sidereq in the _done function. Therefore, I think it's better to just
>> make that internal, not public.
>
> I was trying to retain the _send/_done/_recv structure. The attached
> patch removes the _done function in favor of a hardcoded one that just
> frees the sidereq.
>
>> Also, please put back the comment about
>> how we're freeing it because the callbacks have already been invoked.
>>
>
> Done.
>
>> If you're switching to using an enum for sss_dp_type, please remove the
>> default: section from the switch statement in sss_dp_get_account_msg().
>> I'd prefer that to be a compile-time warning (since most of us built
>> with -Werror, that would make it easy to catch).
>
> Yes, that was the reason I switched it to enum but forgot to actually
> remove the default :-)
>
>>
>> In sss_dp_issue_request(), when you get HASH_SUCCESS, you're just
>> returning EOK immediately. You need to add this caller to the list of
>> callbacks. Presumably you intended to break here instead, like in the
>> original code.
>>
>
> Thanks, fixed
>
>> There's a flaw in your hash key algorithm. It's theoretically possible
>> for two different wrappers to call sss_dp_issue_request() with the same
>> key, but a different msg_create argument. This could result in the hash
>> comparison falsely believing that an existing request of the same type
>> is in progress, whereas it is actually looking up other data. I realize
>> that our current code isn't going to be able to hit this, but we need to
>> account for the possibility. I'd suggest that we might want to build the
>> hash key internal to sss_dp_issue_request() and concatenate it to the
>> pointer address of the msg_create function plus a string key argument
>> (instead of a hash_key_t). This way, we know for certain that it is two
>> requests with the same arguments that are expecting the same reply. So
>> hash keys would look like: "0xdeadbeef:1:jhrozek at mydomain" (for an
>> SSS_DP_USER request against all domains with no 'extra' argument).
>>
>
> Fixed.
>
>> Typo: "DEBUG(SSSDBG_CRIT_FAILURE, ("The request has disappearead?\n"));"
>> Should be: "DEBUG(SSSDBG_CRIT_FAILURE, ("The request has
>> disappeared?\n"));
>>
>
> Fixed.
>
>> While we're making such sweeping changes, could we consider renaming the
>> return values err_maj and err_min to "dp_err" and "errno", respectively?
>> (This isn't mandatory, but it might not be a bad time to do so.). It
>> would also be helpful if those labels were used in error messages where
>> the DP response is printed. (Seeing [1][22][(null)] in the debug logs
>> means a trip to the source to decode).
>>
>
> I changed the names to dp_err and dp_ret to avoid name-clash with errno
> from errno.h
>
>> In general, I think this is a great idea and a terrific refactor. We
>> just need to clean up the above issues.
>>
>
> I also exported sss_dp_issue_request() in responder.h and made it
> non-static as requested in the other mail.
>
> Thank you for the review. A new patch is attached.
>
>
>
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://fedorahosted.org/mailman/listinfo/sssd-devel




More information about the sssd-devel mailing list