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

Pavel Březina pbrezina at redhat.com
Thu Dec 11 13:26:52 UTC 2014


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.




More information about the sssd-devel mailing list