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.