10:45 a.m.
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.
10:24 a.m.
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.
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
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).
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
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.
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.
6:31 a.m.
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.
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
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.
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.
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
12:29 p.m.
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.
>
> 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
>
> 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?).
It also may not be bad to create a ticket to track this.
> 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.
7:52 a.m.
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.
2:21 p.m.
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
6:07 a.m.
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.
9:04 a.m.
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.
Michal
7:06 a.m.
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.
7:26 a.m.
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.
3:31 a.m.
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 :-)
3:53 a.m.
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.
4:02 a.m.
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.
4:24 a.m.
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.
8:12 a.m.
On 01/09/2015 11:24 AM, Jakub Hrozek wrote:
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.
8:19 a.m.
On Fri, Jan 09, 2015 at 03:12:14PM +0100, Pavel Březina wrote:
>>>>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.
* master:
* faae3d55e5cf416f16158d3b9f8c8fd475ac6acf
* 96faa5ca7e395991885f3c354dfe543749b900a7
* 360a4be4266d6a72be99dfd252623dc0527f5b84
* cb4742876508a08ba90c82466c9dba708e4bf999
* 629a188ec71155911301fddc36e360831045d2c6
* acc1c0c07fc76bc05d91c0bd2f172cd638ff3546
* dbb990fb29e7178a3cce53474e48ce69ab3a97a0
3402
days inactive
3466
days old
sssd-devel@lists.fedorahosted.org
15 comments
3 participants
participants (3)
-
Jakub Hrozek -
Michal Židek -
Pavel Březina