On 04/06/2016 05:45 PM, Jakub Hrozek wrote:
On Fri, Apr 01, 2016 at 12:07:04PM +0200, Pavel Březina wrote:
https://fedorahosted.org/sssd/wiki/DesignDocs/DataProvider
I will amend the design page later on so I won't comment on everything here.
For your convenience, the text is copied below:
= Data Provider Refactoring =
Related ticket(s):
* https://fedorahosted.org/sssd/ticket/385
I think you can also add https://fedorahosted.org/sssd/ticket/1878 and more
importantly https://fedorahosted.org/sssd/ticket/490. #385 is something
we want to enable with this work.
#490 was already done by Steff, I think we can close it. Or do you disagree?
* support module's contructor and private data shared across target's
initialization functions
* make method handlers pure tevent requests that returns single error code
Currently we return a triple, err_min, err_maj and error string, do you
plan on changing that? (The document says you don't further down, but
this sentence is a bit conflicting IMO..)
Yes, the triplet stays (at least for now). But it will be returned from
_recv function as output data of the tevent request. The tevent request
itself will return just a single value EOK/ERR_OFFLINE/other which are
converted to D-Bus error.
Later, I'd like convert the current handlers to return different type of
data that suits to each handler better (the triplet is mostly usable
only for pam, most of the hanlders do not need to return an value), but
that will require changes in responder so I don't plan to do it know (or
maybe even for 1.14).
=== Implementation steps ===
1. (done) Implement the new data provider interface
If this is done, can we see the code in some WIP branch? I would just
like to catch any design issues as soon as possible and looking at the
actual code might help.
Yes. See my repo, #backend branch.
https://fedorapeople.org/cgit/pbrezina/public_git/sssd.git/log/?h=backend
I did lots of changes in comparison with the first version I sent.
The changes cover:
* request chaining, if a request is in process we just attached another
sbus_req where we will send the answer (similar to what we do in responders)
* type safe private data, no longer need for void pointers
This allows data type check (compile time when possible, run time
elsewhere) and makes providing custom reply handlers really simple. This
enables us to use typed private data with type safety checks.
For example the request handler may look like this:
struct tevent_req *
send_fn(TALLOC_CTX *mem_ctx,
struct sdap_id_ctx *id_ctx,
struct access_handler_data *data,
struct dp_params *params);
errno_t
reply_fn(TALLOC_CTX *mem_ctx,
struct tevent_req *req,
struct dp_reply_std *output_data)
And the reply function that sends data back to responder:
void dp_req_reply_std(const char *request_name,
struct sbus_request *sbus_req,
struct dp_reply_std *reply);
The output data here in example is actually the current triplet:
struct dp_reply_std {
int dp_error;
int error;
const char *message;
};
So we don't longer need to cast void pointers and we ensure that we pass
correct types.
The dp_params structure contain additional parameters that will be
probably useless most of the time and they are added to struct so we can
simple add more if needed without the need to change everything. This
was actually Pavel Reichl's idea, which I think is great!
2. (wip) Convert modules init functions
3. (wip) Convert existing handlers into tevent requests
4. Switch to the new interface
5. Add new methods and interfaces as needed
=== Responders ===
In the first stage no change to the responders needs to be done.
Not even the responder_common part? That would be nice, because there we
have quite a lot of code that is not tested, but important, such as
chaining duplicate requests etc. I wouldn't like to change this code
without tests..
Not even responder_common part. The only change that needs to be done is
in client initialization code, but that is only interface name.
=== How To Test ===
All existing test must pass and no functionality is broken.
This is hard to say without seeing the code, but the existing code, at
least in the responder, as I said above, covers a number of use-cases
that ordinary tests might not catch, so we might want to work on good
unit tests..
it is simple to say - it's really mandatory that all tests pass and no
functionality is broken :-)
No changes in the responder will be done. But we can definitely write
some unit test and I'd accept every help here, since it's lot's of code.
I think that data provider should be more unit-testable now and someone
can start writing tests now for some parts.
_______________________________________________
sssd-devel mailing list
[email protected]
https://lists.fedorahosted.org/admin/lists/[email protected]
