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

Michal Židek mzidek at redhat.com
Mon Nov 24 20:21:22 UTC 2014


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



More information about the sssd-devel mailing list