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.