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

Michal Židek mzidek at redhat.com
Wed Nov 19 16:24:01 UTC 2014


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.

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
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).

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

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.

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.



More information about the sssd-devel mailing list