This small series contain some really small refactoring work in the save_user() and save_group() functions, as suggested by Lukaš Slebodnik[0].
CI has not passed[1], but the failure occurred just in RHEL-7 in a test that, AFAIR, is failing quite often. So, I'm not completely sure whether it's related or not.
[0]: https://lists.fedorahosted.org/archives/list/sssd-devel@lists.fedorahosted.o... [1]: http://sssd-ci.duckdns.org/logs/job/52/33/summary.html
Best Regards, -- Fabiano Fidêncio
On (24/08/16 15:41), Fabiano Fidêncio wrote:
This small series contain some really small refactoring work in the save_user() and save_group() functions, as suggested by Lukaš Slebodnik[0].
CI has not passed[1], but the failure occurred just in RHEL-7 in a test that, AFAIR, is failing quite often. So, I'm not completely sure whether it's related or not.
Best Regards,
Fabiano Fidêncio
From ffc007cbc5479b60cb2a17556eea183bb9c9de15 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= fidencio@redhat.com Date: Wed, 24 Aug 2016 13:16:31 +0200 Subject: [PATCH 1/5] PROXY: Remove lowercase attribute from save_user() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit
As this function already receives a struct sss_domain_info * parameter as argument, we can simply check whether we will need a lowercase name by accessing domain->case_sensitive.
Related: https://fedorahosted.org/sssd/ticket/3134
Signed-off-by: Fabiano Fidêncio fidencio@redhat.com
src/providers/proxy/proxy_id.c | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-)
LGTM
From fe913fe51812f383846ffe084172e1254de03aca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= fidencio@redhat.com Date: Wed, 24 Aug 2016 13:25:44 +0200 Subject: [PATCH 2/5] PROXY: Remove cache_timeout attribute from save_user() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit
As this function already receives a struct sss_domain_info * parameter as argument, we can simply get the cache_timeout attribute by accessing domain->user_timeout.
Related: https://fedorahosted.org/sssd/ticket/3134
Signed-off-by: Fabiano Fidêncio fidencio@redhat.com
src/providers/proxy/proxy_id.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
ACK
From e8def9c0604f051473a6d57280df02b2297b09f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= fidencio@redhat.com Date: Wed, 24 Aug 2016 13:29:17 +0200 Subject: [PATCH 3/5] PROXY: Remove cache_timeout attribute from save_group() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit
As this function already receives a struct sss_domain_info * parameter as argument, we can simply get the cache_timeout attribute by accessing domain->group_timeout.
Related: https://fedorahosted.org/sssd/ticket/3134
Signed-off-by: Fabiano Fidêncio fidencio@redhat.com
src/providers/proxy/proxy_id.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)
ACK
From 8727e4762dadddc17ffe943e741771b58f1e427a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= fidencio@redhat.com Date: Wed, 24 Aug 2016 13:32:10 +0200 Subject: [PATCH 4/5] PROXY: Mention that save_user()'s parameters are already qualified MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit
Those comments are similar to what we have in the save_group() function.
Related: https://fedorahosted.org/sssd/ticket/3134
Signed-off-by: Fabiano Fidêncio fidencio@redhat.com
src/providers/proxy/proxy_id.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/providers/proxy/proxy_id.c b/src/providers/proxy/proxy_id.c index a500a97..7cae6df 100644 --- a/src/providers/proxy/proxy_id.c +++ b/src/providers/proxy/proxy_id.c @@ -223,8 +223,9 @@ delete_user(struct sss_domain_info *domain, }
static int save_user(struct sss_domain_info *domain,
struct passwd *pwd, const char *real_name,
const char *alias)
struct passwd *pwd,
const char *real_name, /* already qualified */
const char *alias) /* already qualified */
good idea ACK
From 9f92f7b861f4f82f00dd231078a64cc23bc40720 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= fidencio@redhat.com Date: Wed, 24 Aug 2016 14:28:42 +0200 Subject: [PATCH 5/5] PROXY: Share common code of save_{group,user}() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit
These two functions (save_user() and save_group()) share, between themselves, the code preparing the attributes that are going to be stored in the sysdb.
This patch basically splits this code out of those functions and introduces the new prepare_attrs_for_saving_ops().
Related: https://fedorahosted.org/sssd/ticket/3134
Signed-off-by: Fabiano Fidêncio fidencio@redhat.com
src/providers/proxy/proxy_id.c | 142 ++++++++++++++++++----------------------- 1 file changed, 62 insertions(+), 80 deletions(-)
diff --git a/src/providers/proxy/proxy_id.c b/src/providers/proxy/proxy_id.c index 7cae6df..a2782cf 100644 --- a/src/providers/proxy/proxy_id.c +++ b/src/providers/proxy/proxy_id.c @@ -222,49 +222,37 @@ delete_user(struct sss_domain_info *domain, return ret; }
-static int save_user(struct sss_domain_info *domain,
struct passwd *pwd,
const char *real_name, /* already qualified */
const char *alias) /* already qualified */
+static int prepare_attrs_for_saving_ops(TALLOC_CTX *tmp_ctx,
^^^^^^^ tmp_ctx usually means local talloc context inside a function for temporary allocations. If we need allocate result for output argument(return value) then it is called mem_ctx. @see src/providers/ldap/sdap_sudo_refresh.c @see https://talloc.samba.org/talloc/doc/html/libtalloc__bestpractices.html
We internally allocate everything on tmp_ctx and in case of success we steal(talloc_steal) result from tmp_ctx into mem_ctx
bool case_sentitive,
^ typo :-)
const char *real_name, /* already_qualified */
const char *alias, /* already qualified */
struct sysdb_attrs **attrs)
{
- const char *shell;
- const char *gecos;
- struct sysdb_attrs *attrs = NULL;
- errno_t ret;
- const char *cased_alias;
- const char *lc_pw_name = NULL;
- if (pwd->pw_shell && pwd->pw_shell[0] != '\0') {
shell = pwd->pw_shell;
- } else {
shell = NULL;
- }
- if (pwd->pw_gecos && pwd->pw_gecos[0] != '\0') {
gecos = pwd->pw_gecos;
- } else {
gecos = NULL;
- }
- if (!domain->case_sensitive || alias) {
attrs = sysdb_new_attrs(NULL);
if (!attrs) {
DEBUG(SSSDBG_CRIT_FAILURE, "Allocation error ?!\n");
ret = ENOMEM;
goto done;
- const char *lc_name = NULL;
- const char *cased_alias = NULL;
- errno_t ret = EOK;
It's better to not rely that default error is EOK. We usually set "ret = EOK;" before done label.
- if (!case_sentitive || alias == NULL) {
previously there was a "if (!domain->case_sensitive || alias)" So it shoudl be "alias != NULL"
if (*attrs == NULL) {
*attrs = sysdb_new_attrs(tmp_ctx);
We usually do not store anything directly to output argument In most case we use local variable and inside function (it reduces usage of derefference (*attrs) and set resut to output argument before done (togeter with setting ret = EOK
if (*attrs == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "Allocation error ?!\n");
ret = ENOMEM;
goto done;
}} }
- if (!domain->case_sensitive) {
lc_pw_name = sss_tc_utf8_str_tolower(attrs, real_name);
if (lc_pw_name == NULL) {
- if (!case_sentitive) {
lc_name = sss_tc_utf8_str_tolower(*attrs, real_name);
if (lc_name == NULL) { DEBUG(SSSDBG_OP_FAILURE, "Cannot convert name to lowercase.\n"); ret = ENOMEM; goto done; }
ret = sysdb_attrs_add_string(attrs, SYSDB_NAME_ALIAS, lc_pw_name);
if (ret) {
ret = sysdb_attrs_add_string(*attrs, SYSDB_NAME_ALIAS, lc_name);
if (ret != EOK) { DEBUG(SSSDBG_OP_FAILURE, "Could not add name alias\n"); ret = ENOMEM; goto done;
@@ -273,22 +261,54 @@ static int save_user(struct sss_domain_info *domain, }
if (alias) {
cased_alias = sss_get_cased_name(attrs, alias, domain->case_sensitive);
cased_alias = sss_get_cased_name(*attrs, alias, case_sentitive); if (!cased_alias) { ret = ENOMEM; goto done; } /* Add the alias only if it differs from lowercased pw_name */
if (lc_pw_name == NULL || strcmp(cased_alias, lc_pw_name) != 0) {
ret = sysdb_attrs_add_string(attrs, SYSDB_NAME_ALIAS, cased_alias);
if (ret) {
if (lc_name == NULL || strcmp(cased_alias, lc_name) != 0) {
ret = sysdb_attrs_add_string(*attrs, SYSDB_NAME_ALIAS, cased_alias);
}if (ret != EOK) { DEBUG(SSSDBG_OP_FAILURE, "Could not add name alias\n"); goto done; } }
+done:
- return ret;
+}
+static int save_user(struct sss_domain_info *domain,
struct passwd *pwd,
const char *real_name, /* already qualified */
const char *alias) /* already qualified */
+{
- const char *shell;
- const char *gecos;
- struct sysdb_attrs *attrs = NULL;
- errno_t ret;
- if (pwd->pw_shell && pwd->pw_shell[0] != '\0') {
shell = pwd->pw_shell;
- } else {
shell = NULL;
- }
- if (pwd->pw_gecos && pwd->pw_gecos[0] != '\0') {
gecos = pwd->pw_gecos;
- } else {
gecos = NULL;
- }
- ret = prepare_attrs_for_saving_ops(NULL, !domain->case_sensitive,
^^^^ It's better to allocate temporary context rather then directly using NULL.
BTW the 2nd argumet of the function has name "case_sentitive". So I would be better to pass directly domain->case_sensitive there and change logic internaly.
Or we can change the name of parameter in the function. (bool need_lowercase?, lowercas? ...)
Otherwise good job. I did not expect such simplification of code. But I like it.
LS
On Thu, Aug 25, 2016 at 2:07 PM, Lukas Slebodnik lslebodn@redhat.com wrote:
On (24/08/16 15:41), Fabiano Fidêncio wrote:
This small series contain some really small refactoring work in the save_user() and save_group() functions, as suggested by Lukaš Slebodnik[0].
CI has not passed[1], but the failure occurred just in RHEL-7 in a test that, AFAIR, is failing quite often. So, I'm not completely sure whether it's related or not.
Best Regards,
Fabiano Fidêncio
From ffc007cbc5479b60cb2a17556eea183bb9c9de15 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= fidencio@redhat.com Date: Wed, 24 Aug 2016 13:16:31 +0200 Subject: [PATCH 1/5] PROXY: Remove lowercase attribute from save_user() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit
As this function already receives a struct sss_domain_info * parameter as argument, we can simply check whether we will need a lowercase name by accessing domain->case_sensitive.
Related: https://fedorahosted.org/sssd/ticket/3134
Signed-off-by: Fabiano Fidêncio fidencio@redhat.com
src/providers/proxy/proxy_id.c | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-)
LGTM
From fe913fe51812f383846ffe084172e1254de03aca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= fidencio@redhat.com Date: Wed, 24 Aug 2016 13:25:44 +0200 Subject: [PATCH 2/5] PROXY: Remove cache_timeout attribute from save_user() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit
As this function already receives a struct sss_domain_info * parameter as argument, we can simply get the cache_timeout attribute by accessing domain->user_timeout.
Related: https://fedorahosted.org/sssd/ticket/3134
Signed-off-by: Fabiano Fidêncio fidencio@redhat.com
src/providers/proxy/proxy_id.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
ACK
From e8def9c0604f051473a6d57280df02b2297b09f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= fidencio@redhat.com Date: Wed, 24 Aug 2016 13:29:17 +0200 Subject: [PATCH 3/5] PROXY: Remove cache_timeout attribute from save_group() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit
As this function already receives a struct sss_domain_info * parameter as argument, we can simply get the cache_timeout attribute by accessing domain->group_timeout.
Related: https://fedorahosted.org/sssd/ticket/3134
Signed-off-by: Fabiano Fidêncio fidencio@redhat.com
src/providers/proxy/proxy_id.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)
ACK
From 8727e4762dadddc17ffe943e741771b58f1e427a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= fidencio@redhat.com Date: Wed, 24 Aug 2016 13:32:10 +0200 Subject: [PATCH 4/5] PROXY: Mention that save_user()'s parameters are already qualified MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit
Those comments are similar to what we have in the save_group() function.
Related: https://fedorahosted.org/sssd/ticket/3134
Signed-off-by: Fabiano Fidêncio fidencio@redhat.com
src/providers/proxy/proxy_id.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/providers/proxy/proxy_id.c b/src/providers/proxy/proxy_id.c index a500a97..7cae6df 100644 --- a/src/providers/proxy/proxy_id.c +++ b/src/providers/proxy/proxy_id.c @@ -223,8 +223,9 @@ delete_user(struct sss_domain_info *domain, }
static int save_user(struct sss_domain_info *domain,
struct passwd *pwd, const char *real_name,
const char *alias)
struct passwd *pwd,
const char *real_name, /* already qualified */
const char *alias) /* already qualified */
good idea ACK
From 9f92f7b861f4f82f00dd231078a64cc23bc40720 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= fidencio@redhat.com Date: Wed, 24 Aug 2016 14:28:42 +0200 Subject: [PATCH 5/5] PROXY: Share common code of save_{group,user}() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit
These two functions (save_user() and save_group()) share, between themselves, the code preparing the attributes that are going to be stored in the sysdb.
This patch basically splits this code out of those functions and introduces the new prepare_attrs_for_saving_ops().
Related: https://fedorahosted.org/sssd/ticket/3134
Signed-off-by: Fabiano Fidêncio fidencio@redhat.com
src/providers/proxy/proxy_id.c | 142 ++++++++++++++++++----------------------- 1 file changed, 62 insertions(+), 80 deletions(-)
diff --git a/src/providers/proxy/proxy_id.c b/src/providers/proxy/proxy_id.c index 7cae6df..a2782cf 100644 --- a/src/providers/proxy/proxy_id.c +++ b/src/providers/proxy/proxy_id.c @@ -222,49 +222,37 @@ delete_user(struct sss_domain_info *domain, return ret; }
-static int save_user(struct sss_domain_info *domain,
struct passwd *pwd,
const char *real_name, /* already qualified */
const char *alias) /* already qualified */
+static int prepare_attrs_for_saving_ops(TALLOC_CTX *tmp_ctx,
^^^^^^^
tmp_ctx usually means local talloc context inside a function for temporary allocations. If we need allocate result for output argument(return value) then it is called mem_ctx. @see src/providers/ldap/sdap_sudo_refresh.c @see https://talloc.samba.org/talloc/doc/html/libtalloc__bestpractices.html
We internally allocate everything on tmp_ctx and in case of success we steal(talloc_steal) result from tmp_ctx into mem_ctx
Hmm. I see that's something that I should follow but somehow I think it just make the code less readable in this specific case. For instance, I'm passing as an in-out argument the sysdb_attrs, that may or may not have been initialized by the caller. So, I end up re-parenting that memory and then, in the end, may end up moving it back to the original parent.
Anyways, apart from me being or not comfortable with this, I've attached a v2 of the series, which contain a patch that follows your suggestion (on top of my series). Please, if it's okay for you, just squash it in the previous one before pushing.
bool case_sentitive,
^ typo :-)
const char *real_name, /* already_qualified */
const char *alias, /* already qualified */
struct sysdb_attrs **attrs)
{
- const char *shell;
- const char *gecos;
- struct sysdb_attrs *attrs = NULL;
- errno_t ret;
- const char *cased_alias;
- const char *lc_pw_name = NULL;
- if (pwd->pw_shell && pwd->pw_shell[0] != '\0') {
shell = pwd->pw_shell;
- } else {
shell = NULL;
- }
- if (pwd->pw_gecos && pwd->pw_gecos[0] != '\0') {
gecos = pwd->pw_gecos;
- } else {
gecos = NULL;
- }
- if (!domain->case_sensitive || alias) {
attrs = sysdb_new_attrs(NULL);
if (!attrs) {
DEBUG(SSSDBG_CRIT_FAILURE, "Allocation error ?!\n");
ret = ENOMEM;
goto done;
- const char *lc_name = NULL;
- const char *cased_alias = NULL;
- errno_t ret = EOK;
It's better to not rely that default error is EOK. We usually set "ret = EOK;" before done label.
Suggestion taken.
- if (!case_sentitive || alias == NULL) {
previously there was a "if (!domain->case_sensitive || alias)" So it shoudl be "alias != NULL"
if (*attrs == NULL) {
*attrs = sysdb_new_attrs(tmp_ctx);
We usually do not store anything directly to output argument In most case we use local variable and inside function (it reduces usage of derefference (*attrs) and set resut to output argument before done (togeter with setting ret = EOK
Hmm. See my comment above, please.
if (*attrs == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "Allocation error ?!\n");
ret = ENOMEM;
goto done;
}} }
- if (!domain->case_sensitive) {
lc_pw_name = sss_tc_utf8_str_tolower(attrs, real_name);
if (lc_pw_name == NULL) {
- if (!case_sentitive) {
lc_name = sss_tc_utf8_str_tolower(*attrs, real_name);
if (lc_name == NULL) { DEBUG(SSSDBG_OP_FAILURE, "Cannot convert name to lowercase.\n"); ret = ENOMEM; goto done; }
ret = sysdb_attrs_add_string(attrs, SYSDB_NAME_ALIAS, lc_pw_name);
if (ret) {
ret = sysdb_attrs_add_string(*attrs, SYSDB_NAME_ALIAS, lc_name);
if (ret != EOK) { DEBUG(SSSDBG_OP_FAILURE, "Could not add name alias\n"); ret = ENOMEM; goto done;
@@ -273,22 +261,54 @@ static int save_user(struct sss_domain_info *domain, }
if (alias) {
cased_alias = sss_get_cased_name(attrs, alias, domain->case_sensitive);
cased_alias = sss_get_cased_name(*attrs, alias, case_sentitive); if (!cased_alias) { ret = ENOMEM; goto done; } /* Add the alias only if it differs from lowercased pw_name */
if (lc_pw_name == NULL || strcmp(cased_alias, lc_pw_name) != 0) {
ret = sysdb_attrs_add_string(attrs, SYSDB_NAME_ALIAS, cased_alias);
if (ret) {
if (lc_name == NULL || strcmp(cased_alias, lc_name) != 0) {
ret = sysdb_attrs_add_string(*attrs, SYSDB_NAME_ALIAS, cased_alias);
}if (ret != EOK) { DEBUG(SSSDBG_OP_FAILURE, "Could not add name alias\n"); goto done; } }
+done:
- return ret;
+}
+static int save_user(struct sss_domain_info *domain,
struct passwd *pwd,
const char *real_name, /* already qualified */
const char *alias) /* already qualified */
+{
- const char *shell;
- const char *gecos;
- struct sysdb_attrs *attrs = NULL;
- errno_t ret;
- if (pwd->pw_shell && pwd->pw_shell[0] != '\0') {
shell = pwd->pw_shell;
- } else {
shell = NULL;
- }
- if (pwd->pw_gecos && pwd->pw_gecos[0] != '\0') {
gecos = pwd->pw_gecos;
- } else {
gecos = NULL;
- }
- ret = prepare_attrs_for_saving_ops(NULL, !domain->case_sensitive,
^^^^ It's better to allocate temporary context rather then directly using NULL.
BTW the 2nd argumet of the function has name "case_sentitive". So I would be better to pass directly domain->case_sensitive there and change logic internaly.
Or we can change the name of parameter in the function. (bool need_lowercase?, lowercas? ...)
Well, seems that I messed up with this part while doing the rebase, sorry. :-\ Fixed now.
Otherwise good job. I did not expect such simplification of code. But I like it.
Thanks!
Ah, downstream testing are still passing with those patches :-)
One CI test failed on debian, not sure whether it's related though :-\ http://sssd-ci.duckdns.org/logs/job/52/40/summary.html
Best Regards, -- Fabiano Fidêncio
On (26/08/16 08:09), Fabiano Fidêncio wrote:
On Thu, Aug 25, 2016 at 2:07 PM, Lukas Slebodnik lslebodn@redhat.com wrote:
On (24/08/16 15:41), Fabiano Fidêncio wrote:
This small series contain some really small refactoring work in the save_user() and save_group() functions, as suggested by Lukaš Slebodnik[0].
CI has not passed[1], but the failure occurred just in RHEL-7 in a test that, AFAIR, is failing quite often. So, I'm not completely sure whether it's related or not.
Best Regards,
Fabiano Fidêncio
From ffc007cbc5479b60cb2a17556eea183bb9c9de15 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= fidencio@redhat.com Date: Wed, 24 Aug 2016 13:16:31 +0200 Subject: [PATCH 1/5] PROXY: Remove lowercase attribute from save_user() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit
As this function already receives a struct sss_domain_info * parameter as argument, we can simply check whether we will need a lowercase name by accessing domain->case_sensitive.
Related: https://fedorahosted.org/sssd/ticket/3134
Signed-off-by: Fabiano Fidêncio fidencio@redhat.com
src/providers/proxy/proxy_id.c | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-)
LGTM
From fe913fe51812f383846ffe084172e1254de03aca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= fidencio@redhat.com Date: Wed, 24 Aug 2016 13:25:44 +0200 Subject: [PATCH 2/5] PROXY: Remove cache_timeout attribute from save_user() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit
As this function already receives a struct sss_domain_info * parameter as argument, we can simply get the cache_timeout attribute by accessing domain->user_timeout.
Related: https://fedorahosted.org/sssd/ticket/3134
Signed-off-by: Fabiano Fidêncio fidencio@redhat.com
src/providers/proxy/proxy_id.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
ACK
From e8def9c0604f051473a6d57280df02b2297b09f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= fidencio@redhat.com Date: Wed, 24 Aug 2016 13:29:17 +0200 Subject: [PATCH 3/5] PROXY: Remove cache_timeout attribute from save_group() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit
As this function already receives a struct sss_domain_info * parameter as argument, we can simply get the cache_timeout attribute by accessing domain->group_timeout.
Related: https://fedorahosted.org/sssd/ticket/3134
Signed-off-by: Fabiano Fidêncio fidencio@redhat.com
src/providers/proxy/proxy_id.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)
ACK
From 8727e4762dadddc17ffe943e741771b58f1e427a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= fidencio@redhat.com Date: Wed, 24 Aug 2016 13:32:10 +0200 Subject: [PATCH 4/5] PROXY: Mention that save_user()'s parameters are already qualified MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit
Those comments are similar to what we have in the save_group() function.
Related: https://fedorahosted.org/sssd/ticket/3134
Signed-off-by: Fabiano Fidêncio fidencio@redhat.com
src/providers/proxy/proxy_id.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/providers/proxy/proxy_id.c b/src/providers/proxy/proxy_id.c index a500a97..7cae6df 100644 --- a/src/providers/proxy/proxy_id.c +++ b/src/providers/proxy/proxy_id.c @@ -223,8 +223,9 @@ delete_user(struct sss_domain_info *domain, }
static int save_user(struct sss_domain_info *domain,
struct passwd *pwd, const char *real_name,
const char *alias)
struct passwd *pwd,
const char *real_name, /* already qualified */
const char *alias) /* already qualified */
good idea ACK
From 9f92f7b861f4f82f00dd231078a64cc23bc40720 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= fidencio@redhat.com Date: Wed, 24 Aug 2016 14:28:42 +0200 Subject: [PATCH 5/5] PROXY: Share common code of save_{group,user}() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit
These two functions (save_user() and save_group()) share, between themselves, the code preparing the attributes that are going to be stored in the sysdb.
This patch basically splits this code out of those functions and introduces the new prepare_attrs_for_saving_ops().
Related: https://fedorahosted.org/sssd/ticket/3134
Signed-off-by: Fabiano Fidêncio fidencio@redhat.com
src/providers/proxy/proxy_id.c | 142 ++++++++++++++++++----------------------- 1 file changed, 62 insertions(+), 80 deletions(-)
diff --git a/src/providers/proxy/proxy_id.c b/src/providers/proxy/proxy_id.c index 7cae6df..a2782cf 100644 --- a/src/providers/proxy/proxy_id.c +++ b/src/providers/proxy/proxy_id.c @@ -222,49 +222,37 @@ delete_user(struct sss_domain_info *domain, return ret; }
-static int save_user(struct sss_domain_info *domain,
struct passwd *pwd,
const char *real_name, /* already qualified */
const char *alias) /* already qualified */
+static int prepare_attrs_for_saving_ops(TALLOC_CTX *tmp_ctx,
^^^^^^^
tmp_ctx usually means local talloc context inside a function for temporary allocations. If we need allocate result for output argument(return value) then it is called mem_ctx. @see src/providers/ldap/sdap_sudo_refresh.c @see https://talloc.samba.org/talloc/doc/html/libtalloc__bestpractices.html
We internally allocate everything on tmp_ctx and in case of success we steal(talloc_steal) result from tmp_ctx into mem_ctx
Hmm. I see that's something that I should follow but somehow I think it just make the code less readable in this specific case. For instance, I'm passing as an in-out argument the sysdb_attrs, that may or may not have been initialized by the caller. So, I end up re-parenting that memory and then, in the end, may end up moving it back to the original parent.
I didn't notice that it's a in-out variable.
Anyways, apart from me being or not comfortable with this, I've attached a v2 of the series, which contain a patch that follows your suggestion (on top of my series). Please, if it's okay for you, just squash it in the previous one before pushing.
I am not confortable either. We can drop 6th patch.
bool case_sentitive,
^ typo :-)
const char *real_name, /* already_qualified */
const char *alias, /* already qualified */
struct sysdb_attrs **attrs)
{
- const char *shell;
- const char *gecos;
- struct sysdb_attrs *attrs = NULL;
- errno_t ret;
- const char *cased_alias;
- const char *lc_pw_name = NULL;
- if (pwd->pw_shell && pwd->pw_shell[0] != '\0') {
shell = pwd->pw_shell;
- } else {
shell = NULL;
- }
- if (pwd->pw_gecos && pwd->pw_gecos[0] != '\0') {
gecos = pwd->pw_gecos;
- } else {
gecos = NULL;
- }
- if (!domain->case_sensitive || alias) {
attrs = sysdb_new_attrs(NULL);
if (!attrs) {
DEBUG(SSSDBG_CRIT_FAILURE, "Allocation error ?!\n");
ret = ENOMEM;
goto done;
- const char *lc_name = NULL;
- const char *cased_alias = NULL;
- errno_t ret = EOK;
It's better to not rely that default error is EOK. We usually set "ret = EOK;" before done label.
Suggestion taken.
- if (!case_sentitive || alias == NULL) {
previously there was a "if (!domain->case_sensitive || alias)" So it shoudl be "alias != NULL"
if (*attrs == NULL) {
*attrs = sysdb_new_attrs(tmp_ctx);
We usually do not store anything directly to output argument In most case we use local variable and inside function (it reduces usage of derefference (*attrs) and set resut to output argument before done (togeter with setting ret = EOK
Hmm. See my comment above, please.
if (*attrs == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "Allocation error ?!\n");
ret = ENOMEM;
goto done;
}} }
- if (!domain->case_sensitive) {
lc_pw_name = sss_tc_utf8_str_tolower(attrs, real_name);
if (lc_pw_name == NULL) {
- if (!case_sentitive) {
lc_name = sss_tc_utf8_str_tolower(*attrs, real_name);
if (lc_name == NULL) { DEBUG(SSSDBG_OP_FAILURE, "Cannot convert name to lowercase.\n"); ret = ENOMEM; goto done; }
ret = sysdb_attrs_add_string(attrs, SYSDB_NAME_ALIAS, lc_pw_name);
if (ret) {
ret = sysdb_attrs_add_string(*attrs, SYSDB_NAME_ALIAS, lc_name);
if (ret != EOK) { DEBUG(SSSDBG_OP_FAILURE, "Could not add name alias\n"); ret = ENOMEM; goto done;
@@ -273,22 +261,54 @@ static int save_user(struct sss_domain_info *domain, }
if (alias) {
cased_alias = sss_get_cased_name(attrs, alias, domain->case_sensitive);
cased_alias = sss_get_cased_name(*attrs, alias, case_sentitive); if (!cased_alias) { ret = ENOMEM; goto done; } /* Add the alias only if it differs from lowercased pw_name */
if (lc_pw_name == NULL || strcmp(cased_alias, lc_pw_name) != 0) {
ret = sysdb_attrs_add_string(attrs, SYSDB_NAME_ALIAS, cased_alias);
if (ret) {
if (lc_name == NULL || strcmp(cased_alias, lc_name) != 0) {
ret = sysdb_attrs_add_string(*attrs, SYSDB_NAME_ALIAS, cased_alias);
}if (ret != EOK) { DEBUG(SSSDBG_OP_FAILURE, "Could not add name alias\n"); goto done; } }
+done:
- return ret;
+}
+static int save_user(struct sss_domain_info *domain,
struct passwd *pwd,
const char *real_name, /* already qualified */
const char *alias) /* already qualified */
+{
- const char *shell;
- const char *gecos;
- struct sysdb_attrs *attrs = NULL;
- errno_t ret;
- if (pwd->pw_shell && pwd->pw_shell[0] != '\0') {
shell = pwd->pw_shell;
- } else {
shell = NULL;
- }
- if (pwd->pw_gecos && pwd->pw_gecos[0] != '\0') {
gecos = pwd->pw_gecos;
- } else {
gecos = NULL;
- }
- ret = prepare_attrs_for_saving_ops(NULL, !domain->case_sensitive,
^^^^ It's better to allocate temporary context rather then directly using NULL.
BTW the 2nd argumet of the function has name "case_sentitive". So I would be better to pass directly domain->case_sensitive there and change logic internaly.
Or we can change the name of parameter in the function. (bool need_lowercase?, lowercas? ...)
Well, seems that I messed up with this part while doing the rebase, sorry. :-\ Fixed now.
Otherwise good job. I did not expect such simplification of code. But I like it.
Thanks!
Ah, downstream testing are still passing with those patches :-)
One CI test failed on debian, not sure whether it's related though :-\ http://sssd-ci.duckdns.org/logs/job/52/40/summary.html
Best Regards,
Fabiano Fidêncio
From ffc007cbc5479b60cb2a17556eea183bb9c9de15 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= fidencio@redhat.com Date: Wed, 24 Aug 2016 13:16:31 +0200 Subject: [PATCH v2 1/6] PROXY: Remove lowercase attribute from save_user() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit
ACK
http://sssd-ci.duckdns.org/logs/job/52/51/summary.html
LS
On (27/08/16 10:44), Lukas Slebodnik wrote:
On (26/08/16 08:09), Fabiano Fidêncio wrote:
On Thu, Aug 25, 2016 at 2:07 PM, Lukas Slebodnik lslebodn@redhat.com wrote:
On (24/08/16 15:41), Fabiano Fidêncio wrote:
This small series contain some really small refactoring work in the save_user() and save_group() functions, as suggested by Lukaš Slebodnik[0].
CI has not passed[1], but the failure occurred just in RHEL-7 in a test that, AFAIR, is failing quite often. So, I'm not completely sure whether it's related or not.
Best Regards,
Fabiano Fidêncio
From ffc007cbc5479b60cb2a17556eea183bb9c9de15 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= fidencio@redhat.com Date: Wed, 24 Aug 2016 13:16:31 +0200 Subject: [PATCH 1/5] PROXY: Remove lowercase attribute from save_user() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit
As this function already receives a struct sss_domain_info * parameter as argument, we can simply check whether we will need a lowercase name by accessing domain->case_sensitive.
Related: https://fedorahosted.org/sssd/ticket/3134
Signed-off-by: Fabiano Fidêncio fidencio@redhat.com
src/providers/proxy/proxy_id.c | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-)
LGTM
From fe913fe51812f383846ffe084172e1254de03aca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= fidencio@redhat.com Date: Wed, 24 Aug 2016 13:25:44 +0200 Subject: [PATCH 2/5] PROXY: Remove cache_timeout attribute from save_user() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit
As this function already receives a struct sss_domain_info * parameter as argument, we can simply get the cache_timeout attribute by accessing domain->user_timeout.
Related: https://fedorahosted.org/sssd/ticket/3134
Signed-off-by: Fabiano Fidêncio fidencio@redhat.com
src/providers/proxy/proxy_id.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
ACK
From e8def9c0604f051473a6d57280df02b2297b09f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= fidencio@redhat.com Date: Wed, 24 Aug 2016 13:29:17 +0200 Subject: [PATCH 3/5] PROXY: Remove cache_timeout attribute from save_group() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit
As this function already receives a struct sss_domain_info * parameter as argument, we can simply get the cache_timeout attribute by accessing domain->group_timeout.
Related: https://fedorahosted.org/sssd/ticket/3134
Signed-off-by: Fabiano Fidêncio fidencio@redhat.com
src/providers/proxy/proxy_id.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)
ACK
From 8727e4762dadddc17ffe943e741771b58f1e427a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= fidencio@redhat.com Date: Wed, 24 Aug 2016 13:32:10 +0200 Subject: [PATCH 4/5] PROXY: Mention that save_user()'s parameters are already qualified MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit
Those comments are similar to what we have in the save_group() function.
Related: https://fedorahosted.org/sssd/ticket/3134
Signed-off-by: Fabiano Fidêncio fidencio@redhat.com
src/providers/proxy/proxy_id.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/providers/proxy/proxy_id.c b/src/providers/proxy/proxy_id.c index a500a97..7cae6df 100644 --- a/src/providers/proxy/proxy_id.c +++ b/src/providers/proxy/proxy_id.c @@ -223,8 +223,9 @@ delete_user(struct sss_domain_info *domain, }
static int save_user(struct sss_domain_info *domain,
struct passwd *pwd, const char *real_name,
const char *alias)
struct passwd *pwd,
const char *real_name, /* already qualified */
const char *alias) /* already qualified */
good idea ACK
From 9f92f7b861f4f82f00dd231078a64cc23bc40720 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= fidencio@redhat.com Date: Wed, 24 Aug 2016 14:28:42 +0200 Subject: [PATCH 5/5] PROXY: Share common code of save_{group,user}() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit
These two functions (save_user() and save_group()) share, between themselves, the code preparing the attributes that are going to be stored in the sysdb.
This patch basically splits this code out of those functions and introduces the new prepare_attrs_for_saving_ops().
Related: https://fedorahosted.org/sssd/ticket/3134
Signed-off-by: Fabiano Fidêncio fidencio@redhat.com
src/providers/proxy/proxy_id.c | 142 ++++++++++++++++++----------------------- 1 file changed, 62 insertions(+), 80 deletions(-)
diff --git a/src/providers/proxy/proxy_id.c b/src/providers/proxy/proxy_id.c index 7cae6df..a2782cf 100644 --- a/src/providers/proxy/proxy_id.c +++ b/src/providers/proxy/proxy_id.c @@ -222,49 +222,37 @@ delete_user(struct sss_domain_info *domain, return ret; }
-static int save_user(struct sss_domain_info *domain,
struct passwd *pwd,
const char *real_name, /* already qualified */
const char *alias) /* already qualified */
+static int prepare_attrs_for_saving_ops(TALLOC_CTX *tmp_ctx,
^^^^^^^
tmp_ctx usually means local talloc context inside a function for temporary allocations. If we need allocate result for output argument(return value) then it is called mem_ctx. @see src/providers/ldap/sdap_sudo_refresh.c @see https://talloc.samba.org/talloc/doc/html/libtalloc__bestpractices.html
We internally allocate everything on tmp_ctx and in case of success we steal(talloc_steal) result from tmp_ctx into mem_ctx
Hmm. I see that's something that I should follow but somehow I think it just make the code less readable in this specific case. For instance, I'm passing as an in-out argument the sysdb_attrs, that may or may not have been initialized by the caller. So, I end up re-parenting that memory and then, in the end, may end up moving it back to the original parent.
I didn't notice that it's a in-out variable.
Anyways, apart from me being or not comfortable with this, I've attached a v2 of the series, which contain a patch that follows your suggestion (on top of my series). Please, if it's okay for you, just squash it in the previous one before pushing.
I am not confortable either. We can drop 6th patch.
bool case_sentitive,
^ typo :-)
const char *real_name, /* already_qualified */
const char *alias, /* already qualified */
struct sysdb_attrs **attrs)
{
- const char *shell;
- const char *gecos;
- struct sysdb_attrs *attrs = NULL;
- errno_t ret;
- const char *cased_alias;
- const char *lc_pw_name = NULL;
- if (pwd->pw_shell && pwd->pw_shell[0] != '\0') {
shell = pwd->pw_shell;
- } else {
shell = NULL;
- }
- if (pwd->pw_gecos && pwd->pw_gecos[0] != '\0') {
gecos = pwd->pw_gecos;
- } else {
gecos = NULL;
- }
- if (!domain->case_sensitive || alias) {
attrs = sysdb_new_attrs(NULL);
if (!attrs) {
DEBUG(SSSDBG_CRIT_FAILURE, "Allocation error ?!\n");
ret = ENOMEM;
goto done;
- const char *lc_name = NULL;
- const char *cased_alias = NULL;
- errno_t ret = EOK;
It's better to not rely that default error is EOK. We usually set "ret = EOK;" before done label.
Suggestion taken.
- if (!case_sentitive || alias == NULL) {
previously there was a "if (!domain->case_sensitive || alias)" So it shoudl be "alias != NULL"
if (*attrs == NULL) {
*attrs = sysdb_new_attrs(tmp_ctx);
We usually do not store anything directly to output argument In most case we use local variable and inside function (it reduces usage of derefference (*attrs) and set resut to output argument before done (togeter with setting ret = EOK
Hmm. See my comment above, please.
if (*attrs == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "Allocation error ?!\n");
ret = ENOMEM;
goto done;
}} }
- if (!domain->case_sensitive) {
lc_pw_name = sss_tc_utf8_str_tolower(attrs, real_name);
if (lc_pw_name == NULL) {
- if (!case_sentitive) {
lc_name = sss_tc_utf8_str_tolower(*attrs, real_name);
if (lc_name == NULL) { DEBUG(SSSDBG_OP_FAILURE, "Cannot convert name to lowercase.\n"); ret = ENOMEM; goto done; }
ret = sysdb_attrs_add_string(attrs, SYSDB_NAME_ALIAS, lc_pw_name);
if (ret) {
ret = sysdb_attrs_add_string(*attrs, SYSDB_NAME_ALIAS, lc_name);
if (ret != EOK) { DEBUG(SSSDBG_OP_FAILURE, "Could not add name alias\n"); ret = ENOMEM; goto done;
@@ -273,22 +261,54 @@ static int save_user(struct sss_domain_info *domain, }
if (alias) {
cased_alias = sss_get_cased_name(attrs, alias, domain->case_sensitive);
cased_alias = sss_get_cased_name(*attrs, alias, case_sentitive); if (!cased_alias) { ret = ENOMEM; goto done; } /* Add the alias only if it differs from lowercased pw_name */
if (lc_pw_name == NULL || strcmp(cased_alias, lc_pw_name) != 0) {
ret = sysdb_attrs_add_string(attrs, SYSDB_NAME_ALIAS, cased_alias);
if (ret) {
if (lc_name == NULL || strcmp(cased_alias, lc_name) != 0) {
ret = sysdb_attrs_add_string(*attrs, SYSDB_NAME_ALIAS, cased_alias);
}if (ret != EOK) { DEBUG(SSSDBG_OP_FAILURE, "Could not add name alias\n"); goto done; } }
+done:
- return ret;
+}
+static int save_user(struct sss_domain_info *domain,
struct passwd *pwd,
const char *real_name, /* already qualified */
const char *alias) /* already qualified */
+{
- const char *shell;
- const char *gecos;
- struct sysdb_attrs *attrs = NULL;
- errno_t ret;
- if (pwd->pw_shell && pwd->pw_shell[0] != '\0') {
shell = pwd->pw_shell;
- } else {
shell = NULL;
- }
- if (pwd->pw_gecos && pwd->pw_gecos[0] != '\0') {
gecos = pwd->pw_gecos;
- } else {
gecos = NULL;
- }
- ret = prepare_attrs_for_saving_ops(NULL, !domain->case_sensitive,
^^^^ It's better to allocate temporary context rather then directly using NULL.
BTW the 2nd argumet of the function has name "case_sentitive". So I would be better to pass directly domain->case_sensitive there and change logic internaly.
Or we can change the name of parameter in the function. (bool need_lowercase?, lowercas? ...)
Well, seems that I messed up with this part while doing the rebase, sorry. :-\ Fixed now.
Otherwise good job. I did not expect such simplification of code. But I like it.
Thanks!
Ah, downstream testing are still passing with those patches :-)
One CI test failed on debian, not sure whether it's related though :-\ http://sssd-ci.duckdns.org/logs/job/52/40/summary.html
Best Regards,
Fabiano Fidêncio
From ffc007cbc5479b60cb2a17556eea183bb9c9de15 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= fidencio@redhat.com Date: Wed, 24 Aug 2016 13:16:31 +0200 Subject: [PATCH v2 1/6] PROXY: Remove lowercase attribute from save_user() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit
ACK
master: * 69e8b7fcb9e3dc814a9ffc2a97fa656521cc4505 * 9900d2b153ebb7d994ccd05275f18b973556d5b3 * 221d70ae3c5b7bc7384f57ffd3f88f89a3e6ae6a * 2537fe318a3866780abca100cf6eb7c258f9d02b * 413aef1529fb3d5ed4d0f38e219f5456d7fe3ae0
LS
sssd-devel@lists.fedorahosted.org