3:08 p.m.
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.
7:14 p.m.
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.
4:29 a.m.
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.
6:53 a.m.
On Mon, 2012-01-23 at 11:29 +0100, Jakub Hrozek wrote:
I also exported sss_dp_issue_request() in responder.h and made it
non-static as requested in the other mail.
I think you forgot to squash in the responder.h prototype for
sss_dp_issue_request(). Otherwise, this would be an ack (pending push of
the preliminary services code).
7:18 a.m.
On Mon, 2012-01-23 at 07:53 -0500, Stephen Gallagher wrote:
On Mon, 2012-01-23 at 11:29 +0100, Jakub Hrozek wrote:
> I also exported sss_dp_issue_request() in responder.h and made it
> non-static as requested in the other mail.
>
I think you forgot to squash in the responder.h prototype for
sss_dp_issue_request(). Otherwise, this would be an ack (pending push of
the preliminary services code).
Disregard that nack. I misread the patch.
This patch is acked, pending the services patches.
I'd like to get those reviewed today so I can push all of this to master
ASAP. That way we won't be stepping on each others' toes.
9:54 a.m.
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
11:31 a.m.
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.
>
>
11:55 a.m.
Dne 23.1.2012 18:31, Jakub Hrozek napsal(a):
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.
Ack.
8:03 a.m.
On Mon, 2012-01-23 at 18:55 +0100, Pavel Březina wrote:
Dne 23.1.2012 18:31, Jakub Hrozek napsal(a):
> 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.
Ack.
Pushed to master.
7:24 p.m.
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.
One more comment: don't make sss_dp_issue_request() static. It would be
nice if we could build our responder-specific wrappers in separate files
(see my comments on the "SUDO: Provide a sudo DP request based on the
internal_req").
4466
days inactive
4471
days old
sssd-devel@lists.fedorahosted.org
9 comments
3 participants
participants (3)
-
Jakub Hrozek -
Pavel Březina -
Stephen Gallagher