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