On Mon, Jan 23, 2012 at 04:54:10PM +0100, Pavel Březina wrote:
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.
That's a great point.
New patch is attached.
>
>>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.
>
>