On Thu, Aug 25, 2016 at 2:07 PM, Lukas Slebodnik <lsleb...@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.
>>
>>[0]:
>>https://lists.fedorahosted.org/archives/list/sssd-devel@lists.fedorahosted.org/message/75ZRBKEDQVM75LUIDDLFDNLHUHJDZBNF/
>>[1]: http://sssd-ci.duckdns.org/logs/job/52/33/summary.html
>>
>>Best Regards,
>>--
>>Fabiano Fidêncio
>
> >From ffc007cbc5479b60cb2a17556eea183bb9c9de15 Mon Sep 17 00:00:00 2001
>>From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fiden...@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 <fiden...@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?= <fiden...@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 <fiden...@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?= <fiden...@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 <fiden...@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?= <fiden...@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 <fiden...@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?= <fiden...@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 <fiden...@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
From ffc007cbc5479b60cb2a17556eea183bb9c9de15 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fiden...@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
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 <fiden...@redhat.com>
---
src/providers/proxy/proxy_id.c | 22 +++++++++-------------
1 file changed, 9 insertions(+), 13 deletions(-)
diff --git a/src/providers/proxy/proxy_id.c b/src/providers/proxy/proxy_id.c
index b0c8280..ff2631c 100644
--- a/src/providers/proxy/proxy_id.c
+++ b/src/providers/proxy/proxy_id.c
@@ -31,7 +31,7 @@
/* =Getpwnam-wrapper======================================================*/
static int save_user(struct sss_domain_info *domain,
- bool lowercase, struct passwd *pwd, const char *real_name,
+ struct passwd *pwd, const char *real_name,
const char *alias, uint64_t cache_timeout);
static int
@@ -143,8 +143,7 @@ static int get_pw_name(struct proxy_id_ctx *ctx,
}
/* Both lookups went fine, we can save the user now */
- ret = save_user(dom, !dom->case_sensitive, pwd,
- real_name, i_name, dom->user_timeout);
+ ret = save_user(dom, pwd, real_name, i_name, dom->user_timeout);
done:
talloc_zfree(tmpctx);
@@ -224,7 +223,7 @@ delete_user(struct sss_domain_info *domain,
}
static int save_user(struct sss_domain_info *domain,
- bool lowercase, struct passwd *pwd, const char *real_name,
+ struct passwd *pwd, const char *real_name,
const char *alias, uint64_t cache_timeout)
{
const char *shell;
@@ -246,7 +245,7 @@ static int save_user(struct sss_domain_info *domain,
gecos = NULL;
}
- if (lowercase || alias) {
+ if (!domain->case_sensitive || alias) {
attrs = sysdb_new_attrs(NULL);
if (!attrs) {
DEBUG(SSSDBG_CRIT_FAILURE, "Allocation error ?!\n");
@@ -255,7 +254,7 @@ static int save_user(struct sss_domain_info *domain,
}
}
- if (lowercase) {
+ if (!domain->case_sensitive) {
lc_pw_name = sss_tc_utf8_str_tolower(attrs, real_name);
if (lc_pw_name == NULL) {
DEBUG(SSSDBG_OP_FAILURE, "Cannot convert name to lowercase.\n");
@@ -273,7 +272,7 @@ static int save_user(struct sss_domain_info *domain,
}
if (alias) {
- cased_alias = sss_get_cased_name(attrs, alias, !lowercase);
+ cased_alias = sss_get_cased_name(attrs, alias, domain->case_sensitive);
if (!cased_alias) {
ret = ENOMEM;
goto done;
@@ -366,8 +365,7 @@ static int get_pw_uid(struct proxy_id_ctx *ctx,
pwd->pw_name);
goto done;
}
- ret = save_user(dom, !dom->case_sensitive, pwd,
- name, NULL, dom->user_timeout);
+ ret = save_user(dom, pwd, name, NULL, dom->user_timeout);
done:
talloc_zfree(tmpctx);
@@ -497,8 +495,7 @@ static int enum_users(TALLOC_CTX *mem_ctx,
pwd->pw_name);
goto done;
}
- ret = save_user(dom, !dom->case_sensitive, pwd,
- name, NULL, dom->user_timeout);
+ ret = save_user(dom, pwd, name, NULL, dom->user_timeout);
if (ret) {
/* Do not fail completely on errors.
* Just report the failure to save and go on */
@@ -1331,8 +1328,7 @@ static int get_initgr(TALLOC_CTX *mem_ctx,
goto done;
}
- ret = save_user(dom, !dom->case_sensitive, pwd,
- real_name, i_name, dom->user_timeout);
+ ret = save_user(dom, pwd, real_name, i_name, dom->user_timeout);
if (ret) {
DEBUG(SSSDBG_OP_FAILURE, "Could not save user\n");
goto fail;
--
2.7.4
From fe913fe51812f383846ffe084172e1254de03aca Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fiden...@redhat.com>
Date: Wed, 24 Aug 2016 13:25:44 +0200
Subject: [PATCH v2 2/6] 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 <fiden...@redhat.com>
---
src/providers/proxy/proxy_id.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/src/providers/proxy/proxy_id.c b/src/providers/proxy/proxy_id.c
index ff2631c..bdcac66 100644
--- a/src/providers/proxy/proxy_id.c
+++ b/src/providers/proxy/proxy_id.c
@@ -32,7 +32,7 @@
static int save_user(struct sss_domain_info *domain,
struct passwd *pwd, const char *real_name,
- const char *alias, uint64_t cache_timeout);
+ const char *alias);
static int
handle_getpw_result(enum nss_status status, struct passwd *pwd,
@@ -143,7 +143,7 @@ static int get_pw_name(struct proxy_id_ctx *ctx,
}
/* Both lookups went fine, we can save the user now */
- ret = save_user(dom, pwd, real_name, i_name, dom->user_timeout);
+ ret = save_user(dom, pwd, real_name, i_name);
done:
talloc_zfree(tmpctx);
@@ -224,7 +224,7 @@ 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, uint64_t cache_timeout)
+ const char *alias)
{
const char *shell;
const char *gecos;
@@ -299,7 +299,7 @@ static int save_user(struct sss_domain_info *domain,
NULL,
attrs,
NULL,
- cache_timeout,
+ domain->user_timeout,
0);
if (ret) {
DEBUG(SSSDBG_OP_FAILURE, "Could not add user to cache\n");
@@ -365,7 +365,7 @@ static int get_pw_uid(struct proxy_id_ctx *ctx,
pwd->pw_name);
goto done;
}
- ret = save_user(dom, pwd, name, NULL, dom->user_timeout);
+ ret = save_user(dom, pwd, name, NULL);
done:
talloc_zfree(tmpctx);
@@ -495,7 +495,7 @@ static int enum_users(TALLOC_CTX *mem_ctx,
pwd->pw_name);
goto done;
}
- ret = save_user(dom, pwd, name, NULL, dom->user_timeout);
+ ret = save_user(dom, pwd, name, NULL);
if (ret) {
/* Do not fail completely on errors.
* Just report the failure to save and go on */
@@ -1328,7 +1328,7 @@ static int get_initgr(TALLOC_CTX *mem_ctx,
goto done;
}
- ret = save_user(dom, pwd, real_name, i_name, dom->user_timeout);
+ ret = save_user(dom, pwd, real_name, i_name);
if (ret) {
DEBUG(SSSDBG_OP_FAILURE, "Could not save user\n");
goto fail;
--
2.7.4
From e8def9c0604f051473a6d57280df02b2297b09f9 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fiden...@redhat.com>
Date: Wed, 24 Aug 2016 13:29:17 +0200
Subject: [PATCH v2 3/6] 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 <fiden...@redhat.com>
---
src/providers/proxy/proxy_id.c | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/src/providers/proxy/proxy_id.c b/src/providers/proxy/proxy_id.c
index bdcac66..a500a97 100644
--- a/src/providers/proxy/proxy_id.c
+++ b/src/providers/proxy/proxy_id.c
@@ -558,8 +558,7 @@ static errno_t proxy_process_missing_users(struct sysdb_ctx *sysdb,
static int save_group(struct sysdb_ctx *sysdb, struct sss_domain_info *dom,
struct group *grp,
const char *real_name, /* already qualified */
- const char *alias, /* already qualified */
- uint64_t cache_timeout)
+ const char *alias) /* already qualified */
{
errno_t ret, sret;
struct sysdb_attrs *attrs = NULL;
@@ -664,7 +663,7 @@ static int save_group(struct sysdb_ctx *sysdb, struct sss_domain_info *dom,
real_name,
grp->gr_gid,
attrs,
- cache_timeout,
+ dom->group_timeout,
now);
if (ret) {
DEBUG(SSSDBG_OP_FAILURE, "Could not add group to cache\n");
@@ -947,7 +946,7 @@ static int get_gr_name(struct proxy_id_ctx *ctx,
goto done;
}
- ret = save_group(sysdb, dom, grp, real_name, i_name, dom->group_timeout);
+ ret = save_group(sysdb, dom, grp, real_name, i_name);
if (ret) {
DEBUG(SSSDBG_OP_FAILURE,
"Cannot save group [%d]: %s\n", ret, strerror(ret));
@@ -1032,7 +1031,7 @@ static int get_gr_gid(TALLOC_CTX *mem_ctx,
goto done;
}
- ret = save_group(sysdb, dom, grp, name, NULL, dom->group_timeout);
+ ret = save_group(sysdb, dom, grp, name, NULL);
if (ret) {
DEBUG(SSSDBG_OP_FAILURE,
"Cannot save user [%d]: %s\n", ret, strerror(ret));
@@ -1165,8 +1164,7 @@ static int enum_groups(TALLOC_CTX *mem_ctx,
"Ignoring\n");
ret = ENOMEM;
}
- ret = save_group(sysdb, dom, grp, name,
- NULL, dom->group_timeout);
+ ret = save_group(sysdb, dom, grp, name, NULL);
if (ret) {
/* Do not fail completely on errors.
* Just report the failure to save and go on */
--
2.7.4
From 8727e4762dadddc17ffe943e741771b58f1e427a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fiden...@redhat.com>
Date: Wed, 24 Aug 2016 13:32:10 +0200
Subject: [PATCH v2 4/6] 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 <fiden...@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 */
{
const char *shell;
const char *gecos;
--
2.7.4
From 3d5d93a11b375b2924726fc12adcdab91ce9c3ce Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fiden...@redhat.com>
Date: Wed, 24 Aug 2016 14:28:42 +0200
Subject: [PATCH v2 5/6] 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 <fiden...@redhat.com>
---
src/providers/proxy/proxy_id.c | 143 ++++++++++++++++++-----------------------
1 file changed, 63 insertions(+), 80 deletions(-)
diff --git a/src/providers/proxy/proxy_id.c b/src/providers/proxy/proxy_id.c
index 7cae6df..5067f0b 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 *mem_ctx,
+ bool case_sentitive,
+ 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;
+ const char *lc_name = NULL;
+ const char *cased_alias = 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;
+ if (!case_sentitive || alias != NULL) {
+ if (*attrs == NULL) {
+ *attrs = sysdb_new_attrs(mem_ctx);
+ 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;
@@ -272,23 +260,56 @@ static int save_user(struct sss_domain_info *domain,
}
- if (alias) {
- cased_alias = sss_get_cased_name(attrs, alias, domain->case_sensitive);
- if (!cased_alias) {
+ if (alias != NULL) {
+ cased_alias = sss_get_cased_name(*attrs, alias, case_sentitive);
+ if (cased_alias == NULL) {
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;
}
}
}
+ ret = EOK;
+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,
+ real_name, alias, &attrs);
+ if (ret != EOK) {
+ goto done;
+ }
+
ret = sysdb_store_user(domain,
real_name,
pwd->pw_passwd,
@@ -563,8 +584,6 @@ static int save_group(struct sysdb_ctx *sysdb, struct sss_domain_info *dom,
{
errno_t ret, sret;
struct sysdb_attrs *attrs = NULL;
- const char *cased_alias;
- const char *lc_gr_name = NULL;
TALLOC_CTX *tmp_ctx;
time_t now = time(NULL);
bool in_transaction = false;
@@ -618,46 +637,10 @@ static int save_group(struct sysdb_ctx *sysdb, struct sss_domain_info *dom,
}
}
- if (dom->case_sensitive == false || alias) {
- if (!attrs) {
- attrs = sysdb_new_attrs(tmp_ctx);
- if (!attrs) {
- DEBUG(SSSDBG_CRIT_FAILURE, "Allocation error ?!\n");
- ret = ENOMEM;
- goto done;
- }
- }
- }
-
- if (dom->case_sensitive == false) {
- lc_gr_name = sss_tc_utf8_str_tolower(attrs, real_name);
- if (lc_gr_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_gr_name);
- if (ret != EOK) {
- goto done;
- }
- }
-
- if (alias) {
- cased_alias = sss_get_cased_name(attrs, alias, dom->case_sensitive);
- if (!cased_alias) {
- ret = ENOMEM;
- DEBUG(SSSDBG_OP_FAILURE, "Could not add name alias\n");
- goto done;
- }
-
- if (lc_gr_name == NULL || strcmp(cased_alias, lc_gr_name)) {
- ret = sysdb_attrs_add_string(attrs, SYSDB_NAME_ALIAS, cased_alias);
- if (ret) {
- DEBUG(SSSDBG_OP_FAILURE, "Could not add name alias\n");
- goto done;
- }
- }
+ ret = prepare_attrs_for_saving_ops(tmp_ctx, dom->case_sensitive,
+ real_name, alias, &attrs);
+ if (ret != EOK) {
+ goto done;
}
ret = sysdb_store_group(dom,
--
2.7.4
From 6b19c76d0ad361a7052cd00b02f6ad8c2df4ccfb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fiden...@redhat.com>
Date: Fri, 26 Aug 2016 00:44:44 +0200
Subject: [PATCH v2 6/6] PROXY: Tentative to make a better usage of
{tmp,mem}_ctx
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
This patch is basically following Lukaš suggestion about the proper way
to deal with tmp and mem TALLOC_CTXs.
This patch _must_ be squashed in the previous one before pushed.
Related:
https://fedorahosted.org/sssd/ticket/3134
---
src/providers/proxy/proxy_id.c | 35 +++++++++++++++++++++++------------
1 file changed, 23 insertions(+), 12 deletions(-)
diff --git a/src/providers/proxy/proxy_id.c b/src/providers/proxy/proxy_id.c
index 5067f0b..dbafb03 100644
--- a/src/providers/proxy/proxy_id.c
+++ b/src/providers/proxy/proxy_id.c
@@ -226,32 +226,40 @@ static int prepare_attrs_for_saving_ops(TALLOC_CTX *mem_ctx,
bool case_sentitive,
const char *real_name, /* already_qualified */
const char *alias, /* already qualified */
- struct sysdb_attrs **attrs)
+ struct sysdb_attrs **_attrs)
{
+ TALLOC_CTX *tmp_ctx;
const char *lc_name = NULL;
const char *cased_alias = NULL;
+ struct sysdb_attrs *attrs;
errno_t ret;
+ tmp_ctx = talloc_new(NULL);
+ if (tmp_ctx == NULL) {
+ return ENOMEM;
+ }
+
if (!case_sentitive || alias != NULL) {
- if (*attrs == NULL) {
- *attrs = sysdb_new_attrs(mem_ctx);
- if (*attrs == NULL) {
- DEBUG(SSSDBG_CRIT_FAILURE, "Allocation error ?!\n");
- ret = ENOMEM;
- goto done;
- }
+ attrs = *_attrs == NULL ?
+ sysdb_new_attrs(tmp_ctx) :
+ talloc_reparent(mem_ctx, tmp_ctx, *_attrs);
+
+ if (attrs == NULL) {
+ DEBUG(SSSDBG_CRIT_FAILURE, "Allocation error ?!\n");
+ ret = ENOMEM;
+ goto done;
}
}
if (!case_sentitive) {
- lc_name = sss_tc_utf8_str_tolower(*attrs, real_name);
+ 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_name);
+ 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;
@@ -261,7 +269,7 @@ static int prepare_attrs_for_saving_ops(TALLOC_CTX *mem_ctx,
}
if (alias != NULL) {
- cased_alias = sss_get_cased_name(*attrs, alias, case_sentitive);
+ cased_alias = sss_get_cased_name(attrs, alias, case_sentitive);
if (cased_alias == NULL) {
ret = ENOMEM;
goto done;
@@ -269,7 +277,7 @@ static int prepare_attrs_for_saving_ops(TALLOC_CTX *mem_ctx,
/* Add the alias only if it differs from lowercased pw_name */
if (lc_name == NULL || strcmp(cased_alias, lc_name) != 0) {
- ret = sysdb_attrs_add_string(*attrs, SYSDB_NAME_ALIAS, cased_alias);
+ 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;
@@ -278,7 +286,10 @@ static int prepare_attrs_for_saving_ops(TALLOC_CTX *mem_ctx,
}
ret = EOK;
+ *_attrs = talloc_steal(mem_ctx, attrs);
+
done:
+ talloc_free(tmp_ctx);
return ret;
}
--
2.7.4
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
