On Wed, Feb 25, 2015 at 03:30:17PM +0100, Pavel Březina wrote:
New patch set is attached, I hoped it covers all the issues. Some of
them
are implemented as separate patch because either is was too complicated to
do the change in the middle of pachset or I wanted the changes to be more
visible.
On 02/17/2015 03:50 PM, Jakub Hrozek wrote:
>> struct tevent_req *
>>+cache_req_group_by_name_send(TALLOC_CTX *mem_ctx,
>>+ struct tevent_context *ev,
>>+ struct resp_ctx *rctx,
>>+ struct sss_nc_ctx *ncache,
>>+ int neg_timeout,
>>+ int cache_refresh_percent,
>>+ const char *domain,
>>+ const char *name)
>>+{
>>+ struct cache_req_input *input;
>>+
>>+ input = cache_req_input_create(mem_ctx, CACHE_REQ_GROUP_BY_NAME, name, 0);
>>+ if (input == NULL) {
>>+ return NULL;
>>+ }
>
>cache_req_input_create can fail for multiple reasons, I think we should
>add a new error code instead of returning NULL. When NULL is returned
>from the send function, it usually is treated as ENOMEM by the caller.
I made cache_req_input_create() to fail only on ENOMEM and move check id==0
to cache_req_send since it is a valid query unlike name==NULL.
Thanks.
>One additional question - are you also planning to add lookups by UPN?
>It seems that's the only functionality not implemented by this shared
>lookup code.
Thank you for reminding me. The original code predates UPN changes. I will
create a patch after this huge patch set it accepted. Do you agree?
Sure. We "just" need to implement the UPN lookups before this shared
code is reused by the NSS responder.
>In general I like the approach you've taken but I think we can make it
>even better. Add another structure like cache_req_dom_ctx and add it inside
>the cache_req_input structure. Always create cache_req_dom_ctx as a
>talloc child of cache_req_input.
>
>This would not only collapse the above talloc_frees into one, but it
>would also nicely separate the per-domain data from per-lookup data and
>avoid mixing data from different domains.
I played with this a little bit but it actually turned out that it makes the
code more complicated. Since you need to have access to both input and
dom_ctx anyway it doesn't really bring anything from above. I don't have a
patch ready since I abandoned it before working completion. Do you want me
to try again and finish it?
If it's too complex then no. I was just trying to prevent a bug I saw
recently in the pam responder (I think?) where we would re-use input
data where we should have used per-domain formatted data.
>I also think dom_name sounds better than safe_name, but meh.
I went with dom_objname since "dom_name" refers to domain name.
The patches now look good to me. I've rebased them on top of the recent
cmocka changes:
https://fedorapeople.org/cgit/jhrozek/public_git/sssd.git/log/?h=review
If you agree with the changes (only trivial changes, since the context
around the cmocka setup function changed), then I'll push the patches.
I'm also waiting for a Coverity check to finish.