On Fri, Jan 09, 2015 at 11:02:58AM +0100, Pavel Březina wrote:
> 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.
Are you sure? CI went fine:
http://sssd-ci.duckdns.org/logs/commit/d2/b76bbc1a45a5e1cd2310f53549e323a...
Also my local tests all pass.
Those patches are good to go. The problem is elsewhere, I'll continue in
a new thread.