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.