On 10/06/2015 03:16 PM, Pavel Březina wrote:
On 10/01/2015 05:06 PM, Sumit Bose wrote:
> On Thu, Sep 17, 2015 at 11:06:30AM +0200, Pavel Březina wrote:
>> On 09/17/2015 10:32 AM, Petr Cech wrote:
>>> Hi Pavel!
>>>
>>> There is some code between my last end and this continuation. I was
>>> read
>>> it and did't find anything wrong.
>>
>> Hi Petr,
>> thank you for you review. New patches are attached. All comments from
>> the
>> previous mail should be addressed.
>>
>
>
> [PATCH 2/3] cache_req: add support for UPN
>
>> diff --git a/src/db/sysdb_search.c b/src/db/sysdb_search.c
>> index
>>
5f33b225a31e1c1add7f776215d500fb09e127ca..a72a8bac594dc10ff9aa337399f27d9936830a8e
>> 100644
>> --- a/src/db/sysdb_search.c
>> +++ b/src/db/sysdb_search.c
>> @@ -295,6 +295,59 @@ static char *enum_filter(TALLOC_CTX *mem_ctx,
>> return filter;
>> }
>>
>> +int sysdb_getpwupn(TALLOC_CTX *mem_ctx,
>> + struct sss_domain_info *domain,
>> + const char *upn,
>> + struct ldb_result **_res)
>> +{
>> + TALLOC_CTX *tmp_ctx;
>> + struct ldb_message *msg;
>> + struct ldb_result *res;
>> + static const char *attrs[] = SYSDB_PW_ATTRS;
>> + errno_t ret;
>> +
>> + tmp_ctx = talloc_new(NULL);
>> + if (tmp_ctx == NULL) {
>> + DEBUG(SSSDBG_CRIT_FAILURE, "talloc_new() failed\n");
>> + return ENOMEM;
>> + }
>> +
>> + ret = sysdb_search_user_by_upn(tmp_ctx, domain, upn, attrs, &msg);
>> + if (ret != EOK && ret != ENOENT) {
>> + DEBUG(SSSDBG_OP_FAILURE, "sysdb_search_user_by_upn()
>> failed.\n");
>> + goto done;
>> + }
>
> The call stack here would be
>
>
sysdb_getpwupn()->sysdb_search_user_by_upn()->sysdb_search_entry()->ldb_search()
>
>
> and the returned results, starting from ldb_search()
>
> ldb_result -> ldb_message (array) -> ldb_message -> ldb_result
>
> I think it would be easier and safe some allocations if it would be
>
> sysdb_search_user_by_upn()->sysdb_getpwupn()->ldb_search() .
>
> Do you think this change makes sense?
Hi, thanks for the review. Done.
Although I created a new function sysdb_search_user_by_upn_res since you
can't pass attrs into sysdb_getpwnupn (unless we are inconsistent with
api).
>
const char *orig_name;
>
> Instead of adding a new member which again will hold the originally
> provided name I would suggest to refactor the change added by
> e87b2a6e94c1066b3044fe683825ff5b4f8716c2 (cache_req: parse input name if
> needed) by introducing a member called e.g. parsed_name which keeps the
> name returned by sss_parse_inp_recv() and return it in cache_req_recv().
>
> In e87b2a6e94c1066b3044fe683825ff5b4f8716c2 it made sense to overwrite
> orig_name because no additional member was needed. But now I think the
> code would be more clear if orig_name will be kept unmodified.
Done in separate patch so the changes are more visible. It can be
squashed to the second patch.
Sumit found that test won't pass. New patchset is attached.