On 12/11/2014 02:06 PM, Jakub Hrozek wrote:
On Thu, Nov 27, 2014 at 04:04:24PM +0100, Michal Židek wrote:
> 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.
Thank you very much for the patches and the review. I would prefer to
push them after we release 1.12.3 to avoid breaking the 1.12.3 release
with any possible regressions.
Yes, I agree.