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 >+ 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 _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
