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.