[SSSD] [PATCH] responder: new interface for cache request

Pavel Březina pbrezina at redhat.com
Fri Nov 21 13:52:00 UTC 2014


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.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-tests-refactor-create_dom_test_ctx.patch
Type: text/x-patch
Size: 10261 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20141121/793f36ad/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-tests-add-create_multidom_test_ctx.patch
Type: text/x-patch
Size: 3877 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20141121/793f36ad/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-tests-add-test_multidom_suite_cleanup.patch
Type: text/x-patch
Size: 3540 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20141121/793f36ad/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-tests-remove-code-duplication-in-single-domain-clean.patch
Type: text/x-patch
Size: 12557 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20141121/793f36ad/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-responders-new-interface-for-cache-request.patch
Type: text/x-patch
Size: 42866 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20141121/793f36ad/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0006-responders-enable-views-in-cache-request.patch
Type: text/x-patch
Size: 2496 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20141121/793f36ad/attachment-0005.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0007-IFP-use-new-cache-interface.patch
Type: text/x-patch
Size: 15740 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20141121/793f36ad/attachment-0006.bin>


More information about the sssd-devel mailing list