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 <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 > master: * 69e8b7fcb9e3dc814a9ffc2a97fa656521cc4505 * 9900d2b153ebb7d994ccd05275f18b973556d5b3 * 221d70ae3c5b7bc7384f57ffd3f88f89a3e6ae6a * 2537fe318a3866780abca100cf6eb7c258f9d02b * 413aef1529fb3d5ed4d0f38e219f5456d7fe3ae0
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
