[SSSD] [PATCH] responder: new interface for cache request

Jakub Hrozek jhrozek at redhat.com
Thu Dec 11 13:06:16 UTC 2014


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.



More information about the sssd-devel mailing list