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.
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).
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. Also, please put back the comment about
how we're freeing it because the callbacks have already been invoked.
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).
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.
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).
Typo: "DEBUG(SSSDBG_CRIT_FAILURE, ("The request has
disappearead?\n"));"
Should be: "DEBUG(SSSDBG_CRIT_FAILURE, ("The request has
disappeared?\n"));
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).
In general, I think this is a great idea and a terrific refactor. We
just need to clean up the above issues.