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@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(a)lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel