On (26/08/16 08:09), Fabiano Fidêncio wrote:
>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.
>
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?= <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
>
ACK
http://sssd-ci.duckdns.org/logs/job/52/51/summary.html
LS
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
