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

Pavel Březina pbrezina at redhat.com
Fri Jan 9 10:02:58 UTC 2015


On 01/09/2015 10:53 AM, Pavel Březina wrote:
> On 01/09/2015 10:31 AM, Jakub Hrozek wrote:
>> On Thu, Dec 11, 2014 at 02:26:52PM +0100, Pavel Březina wrote:
>>> 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.
>>
>> Can you rebase the patches on top of master? Then I'll push :-)
>
> Thanks. Attached.

Hmm, it seems that these patches make server-tests fail for some reason. 
I will look at it.




More information about the sssd-devel mailing list