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.