On 11/25/2014 01:07 PM, Pavel Březina wrote:
On 11/24/2014 09:21 PM, Michal Židek wrote:
> On 11/21/2014 02:52 PM, Pavel Březina wrote:
>> On 11/20/2014 07:29 PM, Michal Židek wrote:
>>> On 11/20/2014 01:31 PM, Pavel BÅezina wrote:
>>>> On 11/19/2014 05:24 PM, Michal Židek wrote:
>>>>> On 11/06/2014 05:45 PM, Pavel BÅezina wrote:
>>>>>> Hi,
>>>>>> this patch set is based on Jakub's original code. The goal
was to
>>>>>> reduce
>>>>>> code duplication among responders - the part that iterates over
>>>>>> domains
>>>>>> and performs a cache lookup and data provider communication if
>>>>>> object is
>>>>>> staled.
>>>>>>
>>>>>> The first three patches are preparation for unit tests to allow
>>>>>> testing
>>>>>> with multi domain environment. The forth patch is the new
interface.
>>>>>> The
>>>>>> fifth patch enables views with this interface - I made it a
separate
>>>>>> commit so the changes can be more easily spotted.
>>>>>>
>>>>>> And finally the last patch make IFP to use this new interface.
Only
>>>>>> IFP
>>>>>> uses it at the moment so we can find potential bugs without
hitting
>>>>>> NSS
>>>>>> and other responders.
>>>>>>
>>>>>
>>>>> Patch 1:
>>>>> Ack
>>>>>
>>>>> Patch 2:
>>>>> ACK.
>>>>> I first thought it may be better not to share id_provider
>>>>> and params among all domains in create_multidom_test_ctx, but
>>>>> it will be probably the most common case, so it is ok. If we
>>>>> later need different paramaters for each domain we can create
>>>>> function like add_domain_to_test_ctx and call it with domain
>>>>> specific params. So the API is good as-is for now and can be
>>>>> expanded easily if needed.
>>>>
>>>> Doing it in more general way would require lots of additional time
>>>> which
>>>> was not worth it given this is the only multi domain test so far.
>>>>
>>>>> Patch 3:
>>>>> There is some code duplication in test_dom_suite_cleanup
>>>>> and its multidom variant. IMO it would be better to make
>>>>> the test_dom_suite_cleanup just wrapper around the
>>>>
>>>> I will look into the code duplication again.
>>>>
>>>>> multidom version. We could change the function to accept
>>>>> domain name instead of sysdb file name. This will require
>>>>> minor changes in places where the function is called
>>>>> (s/TEST_DOM_SYSDB_FILE/TEST_DOM_NAME or similiar).
>>>>
>>>> I wanted to avoid this since that would require to change almost every
>>>> test. But I can do that.
>>>
>>> Yes, I also hesitated a bit when I wrote this request, but it is
>>> just one line change in every test that uses this function and
>>> it will help to remove the code duplication I mentioned above.
>>
>> Done. In a separate patch.
>>
>>>>
>>>>>
>>>>> Patch 4:
>>>>> In Makefile.am there are some trailing whitespaces on lines
>>>>> 2096 and 2103 (I replaced them with dots here):
>>>>>
>>>>> 2094 responder_cache_req_tests_LDFLAGS = \
>>>>> 2095 -Wl,-wrap,sss_dp_get_account_send \
>>>>> 2096 $(NULL)....
>>>>> 2097 responder_cache_req_tests_LDADD = \
>>>>> 2098 $(CMOCKA_LIBS) \
>>>>> 2099 $(SSSD_LIBS) \
>>>>> 2100 $(SSSD_INTERNAL_LTLIBS) \
>>>>> 2101 libsss_test_common.la \
>>>>> 2102 $(NULL)
>>>>> 2103 ....
>>>>> 2104 endif # HAVE_CMOCKA
>>
>> Fixed.
>>
>>>>>
>>>>> Is this new interface meant to be used in IFP only?
>>>>> I guess not. I think it should not be limited to users
>>>>> and initgroups. It would be good to put TODO or FIXME
>>>>> label to the header file to remind us to add support
>>>>> for other objects as well.
>>>>
>>>> The plan is to test it in IFP and expand it to other responders in
>>>> later
>>>> version to be sure we won't be breaking fundamental functionality.
>>>> So it
>>>> will not be limited to users and initgroups in near future. I tried to
>>>> write is as generic as possible to make future extensions easy and
>>>> safe.
>>>
>>> And you did it well, but my point was that the API looks
>>> like it is able to work with all kinds of objects already (from
>>> the names of the functions, nothing indicates that they
>>> are meant for users and initgroups only). That is why I would
>>> like the have TODO/FIXME/NOTE label (probably in the header?).
>>
>> OK.
>>
>>> It also may not be bad to create a ticket to track this.
>>
>> I'll create a ticket when these patches get to the master.
>>
>>>>> In the tests, please use a macro constant for the timeout
>>>>> values (for example in cache_req_user_by_name_send).
>>>>>
>>>>> Otherwise the patch looks good.
>>>>>
>>>>> Patch 5:
>>>>> Ack.
>>>>>
>>>>> Patch 6:
>>>>> Ack.
>>
>
> Hi, sorry for delayed answer. Some nitpicks that caused
> warnings for me:
>
> In src/tests/common_dom.c:
>
> 44
> 45 cdb_path = talloc_asprintf(tmp_ctx, "%s/%s", ...
> 46 if (cdb_path == NULL) {
> 47 DEBUG(SSSDBG_CRIT_FAILURE, "talloc_asprintf failed\n");
> >>>> here should be 'ret = ENOMEM;' <<<<<
> 48 goto done;
> 49 }
>
> And in the same file:
>
> 66 static errno_t
> 67 mock_confdb_domain(TALLOC_CTX *mem_ctx,
> 68 struct confdb_ctx *cdb,
> 69 const char *db_path,
> 70 const char *name,
> 71 const char *id_provider,
> 72 struct sss_test_conf_param *params,
> 73 char **_cdb_path)
> 74 {
> 75 TALLOC_CTX *tmp_ctx = NULL;
> 76 const char *val[2] = {NULL, NULL};
> 77 char *cdb_path = NULL;
> 78 char **array = NULL;
> 79 char *list = NULL;
> 80 bool exists;
> ^^^^^^ initialize this to false
> 81 errno_t ret;
> 82 int i;
>
>
> Michal
Thanks. New patches are attached.
I also removed code duplication between create_dom_test_ctx and
create_multidom_test_ctx which it seems I have forgotten.
Ah, good catch with the create_multidom_test_ctx code duplication.
The code looks good to me now.
ACK to all patches.
Michal