Hi list,
there is patch solving [1] attached.
It is possible that we will have similiar problem with other local keys, so I wrote special function for adding key to list of missing with condition if it is needed or not.
[1] https://fedorahosted.org/sssd/ticket/2841
Regards
On 07/22/2016 02:34 PM, Petr Cech wrote:
Hi list,
there is patch solving [1] attached.
It is possible that we will have similiar problem with other local keys, so I wrote special function for adding key to list of missing with condition if it is needed or not.
[1] https://fedorahosted.org/sssd/ticket/2841
Regards
Hi list,
I am sorry... I forget provide more information about this issue.
I reproduced and tested it with the sssd box connected to the LDAP server which was running as part of FreeIPA server. (The trick is you connect sssd box to the FreeIPA and then you change the configuration of SSSD, so you have right certificate and so on).
After it, I manage netgroups on LDAP via FreeIPA webadmin (this is bonus of this environment).
I create three netgroups ng_[1,2,3] such as ng_1 members were ng_2, ng_3. I could remove one by one group and saw what happend.
Very usefull was command: # sss_cache -N && getent netgroup ng_1 And I can check local db with ldbsearch too.
There were local key 'memberNisNetgroup' which is connected to 'originalMemberNisNetgroup' living in LDAP :-)
This local key didn't disappear when we removed last group. And that was the problem.
Regards
Two nitpicks, see inline.
On 07/22/2016 02:34 PM, Petr Cech wrote:
+static errno_t add_to_missing_attrs (TALLOC_CTX * mem_ctx,
struct sysdb_attrs *attrs,
const char *ext_key,
char ***_missing)
^ Coding style. Remove the space between function name and "(". Do not forget to align the parameters after that.
+{
- bool is_present = false;
- size_t size = 0;
- size_t ret;
- for (int i = 0; i < attrs->num; i++) {
if (strcmp(ext_key, attrs->a[i].name) == 0) {
is_present = true;
}
size++;
- }
- if (is_present == false) {
ret = add_string_to_list(attrs, ext_key, _missing);
if (ret != EOK) {
goto fail;
}
- }
- ret = EOK;
+fail:
Please change the label name to "done". According to our new coding style, the code that follows label "fail" is only executed when failure occurs. I know we do not follow this everywhere, but I would like to be consistent in new code.
- return ret;
+}
Other than that it looks good to me.
Also it would be good to add a CI tests for this. I do not want to postpone this patch before release, so you can do it later as part of this ticket: https://fedorahosted.org/sssd/ticket/3119
So either send a patch with CI test now or assign the above ticket to yourself and do it when there is more time.
Michal
On (03/08/16 12:34), Michal Židek wrote:
Two nitpicks, see inline.
On 07/22/2016 02:34 PM, Petr Cech wrote:
+static errno_t add_to_missing_attrs (TALLOC_CTX * mem_ctx,
struct sysdb_attrs *attrs,
const char *ext_key,
char ***_missing)
^
Coding style. Remove the space between function name and "(". Do not forget to align the parameters after that.
+{
- bool is_present = false;
- size_t size = 0;
- size_t ret;
- for (int i = 0; i < attrs->num; i++) {
if (strcmp(ext_key, attrs->a[i].name) == 0) {
is_present = true;
}
size++;
- }
- if (is_present == false) {
ret = add_string_to_list(attrs, ext_key, _missing);
if (ret != EOK) {
goto fail;
}
- }
- ret = EOK;
+fail:
Please change the label name to "done". According to our new coding style, the code that follows label "fail" is only executed when failure occurs. I know we do not follow this everywhere, but I would like to be consistent in new code.
- return ret;
+}
Other than that it looks good to me.
Also it would be good to add a CI tests for this. I do not want to postpone this patch before release, so you can do it later as part of this ticket: https://fedorahosted.org/sssd/ticket/3119
So either send a patch with CI test now or assign the above ticket to yourself and do it when there is more time.
There is not "either or" for this case :-)
Our experience(e.g. #3045 and many others) in sssd is that if test is not written together with fix then it will be very very difficult to write it later. e.g. There isn't time; there are tasks with higher priority ...
That's the reason why test for this particular bug must be pushed together with the fix.
We should consider ticket #3119 as an enhancement of our testing of netgroups and not as a replacement of testing of this bug.
I hope we are all on the same page.
LS
On 08/03/2016 12:46 PM, Lukas Slebodnik wrote:
On (03/08/16 12:34), Michal Židek wrote:
Two nitpicks, see inline.
On 07/22/2016 02:34 PM, Petr Cech wrote:
+static errno_t add_to_missing_attrs (TALLOC_CTX * mem_ctx,
struct sysdb_attrs *attrs,
const char *ext_key,
char ***_missing)
^
Coding style. Remove the space between function name and "(". Do not forget to align the parameters after that.
+{
- bool is_present = false;
- size_t size = 0;
- size_t ret;
- for (int i = 0; i < attrs->num; i++) {
if (strcmp(ext_key, attrs->a[i].name) == 0) {
is_present = true;
}
size++;
- }
- if (is_present == false) {
ret = add_string_to_list(attrs, ext_key, _missing);
if (ret != EOK) {
goto fail;
}
- }
- ret = EOK;
+fail:
Please change the label name to "done". According to our new coding style, the code that follows label "fail" is only executed when failure occurs. I know we do not follow this everywhere, but I would like to be consistent in new code.
- return ret;
+}
Other than that it looks good to me.
Also it would be good to add a CI tests for this. I do not want to postpone this patch before release, so you can do it later as part of this ticket: https://fedorahosted.org/sssd/ticket/3119
So either send a patch with CI test now or assign the above ticket to yourself and do it when there is more time.
There is not "either or" for this case :-)
Our experience(e.g. #3045 and many others) in sssd is that if test is not written together with fix then it will be very very difficult to write it later. e.g. There isn't time; there are tasks with higher priority ...
That's the reason why test for this particular bug must be pushed together with the fix.
We should consider ticket #3119 as an enhancement of our testing of netgroups and not as a replacement of testing of this bug.
I hope we are all on the same page.
LS
Hi all,
Michal, thanks for review. Lukas, yes, we are on the same page, I hope. Tests are important. I would like to write tests during rest of week.
Regards
On Wed, Aug 03, 2016 at 12:52:31PM +0200, Petr Cech wrote:
On 08/03/2016 12:46 PM, Lukas Slebodnik wrote:
On (03/08/16 12:34), Michal Židek wrote:
Two nitpicks, see inline.
On 07/22/2016 02:34 PM, Petr Cech wrote:
+static errno_t add_to_missing_attrs (TALLOC_CTX * mem_ctx,
struct sysdb_attrs *attrs,
const char *ext_key,
char ***_missing)
^
Coding style. Remove the space between function name and "(". Do not forget to align the parameters after that.
+{
- bool is_present = false;
- size_t size = 0;
- size_t ret;
- for (int i = 0; i < attrs->num; i++) {
if (strcmp(ext_key, attrs->a[i].name) == 0) {
is_present = true;
}
size++;
- }
- if (is_present == false) {
ret = add_string_to_list(attrs, ext_key, _missing);
if (ret != EOK) {
goto fail;
}
- }
- ret = EOK;
+fail:
Please change the label name to "done". According to our new coding style, the code that follows label "fail" is only executed when failure occurs. I know we do not follow this everywhere, but I would like to be consistent in new code.
- return ret;
+}
Other than that it looks good to me.
Also it would be good to add a CI tests for this. I do not want to postpone this patch before release, so you can do it later as part of this ticket: https://fedorahosted.org/sssd/ticket/3119
So either send a patch with CI test now or assign the above ticket to yourself and do it when there is more time.
There is not "either or" for this case :-)
Our experience(e.g. #3045 and many others) in sssd is that if test is not written together with fix then it will be very very difficult to write it later. e.g. There isn't time; there are tasks with higher priority ...
That's the reason why test for this particular bug must be pushed together with the fix.
We should consider ticket #3119 as an enhancement of our testing of netgroups and not as a replacement of testing of this bug.
I hope we are all on the same page.
LS
Hi all,
Michal, thanks for review. Lukas, yes, we are on the same page, I hope. Tests are important. I would like to write tests during rest of week.
I wrote that in the ticket Michal filed, but I don't see support for netgroups in nss_wrapper. But I don't think it should be too difficult to add..
On (03/08/16 13:29), Jakub Hrozek wrote:
On Wed, Aug 03, 2016 at 12:52:31PM +0200, Petr Cech wrote:
On 08/03/2016 12:46 PM, Lukas Slebodnik wrote:
On (03/08/16 12:34), Michal Židek wrote:
Two nitpicks, see inline.
On 07/22/2016 02:34 PM, Petr Cech wrote:
+static errno_t add_to_missing_attrs (TALLOC_CTX * mem_ctx,
struct sysdb_attrs *attrs,
const char *ext_key,
char ***_missing)
^
Coding style. Remove the space between function name and "(". Do not forget to align the parameters after that.
+{
- bool is_present = false;
- size_t size = 0;
- size_t ret;
- for (int i = 0; i < attrs->num; i++) {
if (strcmp(ext_key, attrs->a[i].name) == 0) {
is_present = true;
}
size++;
- }
- if (is_present == false) {
ret = add_string_to_list(attrs, ext_key, _missing);
if (ret != EOK) {
goto fail;
}
- }
- ret = EOK;
+fail:
Please change the label name to "done". According to our new coding style, the code that follows label "fail" is only executed when failure occurs. I know we do not follow this everywhere, but I would like to be consistent in new code.
- return ret;
+}
Other than that it looks good to me.
Also it would be good to add a CI tests for this. I do not want to postpone this patch before release, so you can do it later as part of this ticket: https://fedorahosted.org/sssd/ticket/3119
So either send a patch with CI test now or assign the above ticket to yourself and do it when there is more time.
There is not "either or" for this case :-)
Our experience(e.g. #3045 and many others) in sssd is that if test is not written together with fix then it will be very very difficult to write it later. e.g. There isn't time; there are tasks with higher priority ...
That's the reason why test for this particular bug must be pushed together with the fix.
We should consider ticket #3119 as an enhancement of our testing of netgroups and not as a replacement of testing of this bug.
I hope we are all on the same page.
LS
Hi all,
Michal, thanks for review. Lukas, yes, we are on the same page, I hope. Tests are important. I would like to write tests during rest of week.
I wrote that in the ticket Michal filed, but I don't see support for netgroups in nss_wrapper. But I don't think it should be too difficult to add..
Faster would be to do the same as with initgroups (sssd_id.py) If is is too difficult we can do it as a part of #3119
LS
On 08/03/2016 12:34 PM, Michal Židek wrote:
Two nitpicks, see inline.
On 07/22/2016 02:34 PM, Petr Cech wrote:
+static errno_t add_to_missing_attrs (TALLOC_CTX * mem_ctx,
struct sysdb_attrs *attrs,
const char *ext_key,
char ***_missing)
^
Coding style. Remove the space between function name and "(". Do not forget to align the parameters after that.
Addressed.
+{
- bool is_present = false;
- size_t size = 0;
- size_t ret;
- for (int i = 0; i < attrs->num; i++) {
if (strcmp(ext_key, attrs->a[i].name) == 0) {
is_present = true;
}
size++;
- }
- if (is_present == false) {
ret = add_string_to_list(attrs, ext_key, _missing);
if (ret != EOK) {
goto fail;
}
- }
- ret = EOK;
+fail:
Please change the label name to "done". According to our new coding style, the code that follows label "fail" is only executed when failure occurs. I know we do not follow this everywhere, but I would like to be consistent in new code.
Addressed.
- return ret;
+}
Other than that it looks good to me.
Also it would be good to add a CI tests for this. I do not want to postpone this patch before release, so you can do it later as part of this ticket: https://fedorahosted.org/sssd/ticket/3119
So either send a patch with CI test now or assign the above ticket to yourself and do it when there is more time.
Michal
Hello,
there is fixed patch attached.
I am working on CI test. I hope it will be ready soon.
Regards.
On 08/11/2016 08:53 AM, Petr Cech wrote:
On 08/03/2016 12:34 PM, Michal Židek wrote:
Two nitpicks, see inline.
On 07/22/2016 02:34 PM, Petr Cech wrote:
+static errno_t add_to_missing_attrs (TALLOC_CTX * mem_ctx,
struct sysdb_attrs *attrs,
const char *ext_key,
char ***_missing)
^
Coding style. Remove the space between function name and "(". Do not forget to align the parameters after that.
Addressed.
+{
- bool is_present = false;
- size_t size = 0;
- size_t ret;
- for (int i = 0; i < attrs->num; i++) {
if (strcmp(ext_key, attrs->a[i].name) == 0) {
is_present = true;
}
size++;
- }
- if (is_present == false) {
ret = add_string_to_list(attrs, ext_key, _missing);
if (ret != EOK) {
goto fail;
}
- }
- ret = EOK;
+fail:
Please change the label name to "done". According to our new coding style, the code that follows label "fail" is only executed when failure occurs. I know we do not follow this everywhere, but I would like to be consistent in new code.
Addressed.
- return ret;
+}
Other than that it looks good to me.
Also it would be good to add a CI tests for this. I do not want to postpone this patch before release, so you can do it later as part of this ticket: https://fedorahosted.org/sssd/ticket/3119
So either send a patch with CI test now or assign the above ticket to yourself and do it when there is more time.
Michal
Hello,
there is fixed patch attached.
I am working on CI test. I hope it will be ready soon.
Regards.
CI INTG tests will be solved in another mail thread. They are dependent on Lukas patches.
Regards
On 08/12/2016 09:17 AM, Petr Cech wrote:
On 08/11/2016 08:53 AM, Petr Cech wrote:
On 08/03/2016 12:34 PM, Michal Židek wrote:
Two nitpicks, see inline.
On 07/22/2016 02:34 PM, Petr Cech wrote:
+static errno_t add_to_missing_attrs (TALLOC_CTX * mem_ctx,
struct sysdb_attrs *attrs,
const char *ext_key,
char ***_missing)
^
Coding style. Remove the space between function name and "(". Do not forget to align the parameters after that.
Addressed.
+{
- bool is_present = false;
- size_t size = 0;
- size_t ret;
- for (int i = 0; i < attrs->num; i++) {
if (strcmp(ext_key, attrs->a[i].name) == 0) {
is_present = true;
}
size++;
- }
- if (is_present == false) {
ret = add_string_to_list(attrs, ext_key, _missing);
if (ret != EOK) {
goto fail;
}
- }
- ret = EOK;
+fail:
Please change the label name to "done". According to our new coding style, the code that follows label "fail" is only executed when failure occurs. I know we do not follow this everywhere, but I would like to be consistent in new code.
Addressed.
- return ret;
+}
Other than that it looks good to me.
Also it would be good to add a CI tests for this. I do not want to postpone this patch before release, so you can do it later as part of this ticket: https://fedorahosted.org/sssd/ticket/3119
So either send a patch with CI test now or assign the above ticket to yourself and do it when there is more time.
Michal
Hello,
there is fixed patch attached.
I am working on CI test. I hope it will be ready soon.
Regards.
CI INTG tests will be solved in another mail thread. They are dependent on Lukas patches.
Tests are solved in [SSSD] [PATCH SET] WIP: INTG: Tests for ldap nested netgroups mainling thread.
On 08/17/2016 02:37 PM, Petr Cech wrote:
On 08/12/2016 09:17 AM, Petr Cech wrote:
On 08/11/2016 08:53 AM, Petr Cech wrote:
On 08/03/2016 12:34 PM, Michal Židek wrote:
Two nitpicks, see inline.
On 07/22/2016 02:34 PM, Petr Cech wrote:
+static errno_t add_to_missing_attrs (TALLOC_CTX * mem_ctx,
struct sysdb_attrs *attrs,
const char *ext_key,
char ***_missing)
^
Coding style. Remove the space between function name and "(". Do not forget to align the parameters after that.
Addressed.
+{
- bool is_present = false;
- size_t size = 0;
- size_t ret;
- for (int i = 0; i < attrs->num; i++) {
if (strcmp(ext_key, attrs->a[i].name) == 0) {
is_present = true;
}
size++;
- }
- if (is_present == false) {
ret = add_string_to_list(attrs, ext_key, _missing);
if (ret != EOK) {
goto fail;
}
- }
- ret = EOK;
+fail:
Please change the label name to "done". According to our new coding style, the code that follows label "fail" is only executed when failure occurs. I know we do not follow this everywhere, but I would like to be consistent in new code.
Addressed.
- return ret;
+}
Other than that it looks good to me.
Also it would be good to add a CI tests for this. I do not want to postpone this patch before release, so you can do it later as part of this ticket: https://fedorahosted.org/sssd/ticket/3119
So either send a patch with CI test now or assign the above ticket to yourself and do it when there is more time.
Michal
Hello,
there is fixed patch attached.
I am working on CI test. I hope it will be ready soon.
Regards.
CI INTG tests will be solved in another mail thread. They are dependent on Lukas patches.
Tests are solved in [SSSD] [PATCH SET] WIP: INTG: Tests for ldap nested netgroups mainling thread.
Hello,
whole patch set for LDAP: Fixing of removing netgroup from cache is in [SSSD] [PATCH SET] WIP: INTG: Tests for ldap nested netgroups mailing thread.
On Fri, Aug 19, 2016 at 12:54:07PM +0200, Petr Cech wrote:
On 08/17/2016 02:37 PM, Petr Cech wrote:
On 08/12/2016 09:17 AM, Petr Cech wrote:
On 08/11/2016 08:53 AM, Petr Cech wrote:
On 08/03/2016 12:34 PM, Michal Židek wrote:
Two nitpicks, see inline.
On 07/22/2016 02:34 PM, Petr Cech wrote:
+static errno_t add_to_missing_attrs (TALLOC_CTX * mem_ctx,
struct sysdb_attrs *attrs,
const char *ext_key,
char ***_missing)
^
Coding style. Remove the space between function name and "(". Do not forget to align the parameters after that.
Addressed.
+{
- bool is_present = false;
- size_t size = 0;
- size_t ret;
- for (int i = 0; i < attrs->num; i++) {
if (strcmp(ext_key, attrs->a[i].name) == 0) {
is_present = true;
}
size++;
- }
- if (is_present == false) {
ret = add_string_to_list(attrs, ext_key, _missing);
if (ret != EOK) {
goto fail;
}
- }
- ret = EOK;
+fail:
Please change the label name to "done". According to our new coding style, the code that follows label "fail" is only executed when failure occurs. I know we do not follow this everywhere, but I would like to be consistent in new code.
Addressed.
- return ret;
+}
Other than that it looks good to me.
Also it would be good to add a CI tests for this. I do not want to postpone this patch before release, so you can do it later as part of this ticket: https://fedorahosted.org/sssd/ticket/3119
So either send a patch with CI test now or assign the above ticket to yourself and do it when there is more time.
Michal
Hello,
there is fixed patch attached.
I am working on CI test. I hope it will be ready soon.
Regards.
CI INTG tests will be solved in another mail thread. They are dependent on Lukas patches.
Tests are solved in [SSSD] [PATCH SET] WIP: INTG: Tests for ldap nested netgroups mainling thread.
Hello,
whole patch set for LDAP: Fixing of removing netgroup from cache is in [SSSD] [PATCH SET] WIP: INTG: Tests for ldap nested netgroups mailing thread.
Can you change the thread subject then with the next response there? It's a bit confusing that a fix is in WIP thread whose subject is tests..
On 08/19/2016 01:26 PM, Jakub Hrozek wrote:
On Fri, Aug 19, 2016 at 12:54:07PM +0200, Petr Cech wrote:
On 08/17/2016 02:37 PM, Petr Cech wrote:
On 08/12/2016 09:17 AM, Petr Cech wrote:
On 08/11/2016 08:53 AM, Petr Cech wrote:
On 08/03/2016 12:34 PM, Michal Židek wrote:
Two nitpicks, see inline.
On 07/22/2016 02:34 PM, Petr Cech wrote: > > +static errno_t add_to_missing_attrs (TALLOC_CTX * mem_ctx, > + struct sysdb_attrs *attrs, > + const char *ext_key, > + char ***_missing) ^ Coding style. Remove the space between function name and "(". Do not forget to align the parameters after that.
Addressed.
> +{ > + bool is_present = false; > + size_t size = 0; > + size_t ret; > + > + for (int i = 0; i < attrs->num; i++) { > + if (strcmp(ext_key, attrs->a[i].name) == 0) { > + is_present = true; > + } > + size++; > + } > + > + if (is_present == false) { > + ret = add_string_to_list(attrs, ext_key, _missing); > + if (ret != EOK) { > + goto fail; > + } > + } > + > + ret = EOK; > + > +fail:
Please change the label name to "done". According to our new coding style, the code that follows label "fail" is only executed when failure occurs. I know we do not follow this everywhere, but I would like to be consistent in new code.
Addressed.
> + return ret; > +} > +
Other than that it looks good to me.
Also it would be good to add a CI tests for this. I do not want to postpone this patch before release, so you can do it later as part of this ticket: https://fedorahosted.org/sssd/ticket/3119
So either send a patch with CI test now or assign the above ticket to yourself and do it when there is more time.
Michal
Hello,
there is fixed patch attached.
I am working on CI test. I hope it will be ready soon.
Regards.
CI INTG tests will be solved in another mail thread. They are dependent on Lukas patches.
Tests are solved in [SSSD] [PATCH SET] WIP: INTG: Tests for ldap nested netgroups mainling thread.
Hello,
whole patch set for LDAP: Fixing of removing netgroup from cache is in [SSSD] [PATCH SET] WIP: INTG: Tests for ldap nested netgroups mailing thread.
Can you change the thread subject then with the next response there? It's a bit confusing that a fix is in WIP thread whose subject is tests..
Right, subject changed from [SSSD] [PATCH SET] WIP: INTG: Tests for ldap nested netgroups to [SSSD] [PATCH] LDAP: Fixing of removing netgroup from cache
sssd-devel@lists.fedorahosted.org