On Wed, Jul 13, 2016 at 10:41 AM, Pavel Březina <[email protected]> wrote: > On 07/12/2016 01:49 PM, Fabiano Fidêncio wrote: >> >> On Tue, Jul 12, 2016 at 1:37 PM, Pavel Březina <[email protected]> >> wrote: >>> >>> On 07/12/2016 01:31 PM, Fabiano Fidêncio wrote: >>>> >>>> >>>> Replying with comments in-line: >>>> >>>> From a4a2eb297e6e56e6d0d05ab3810ab87eb320ebdb Mon Sep 17 00:00:00 2001 >>>> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <[email protected]> >>>> Date: Tue, 12 Jul 2016 12:59:48 +0200 >>>> Subject: [PATCH] sssctl: move filter creation to separate function >>>> >>>> --- >>>> src/tools/sssctl/sssctl_cache.c | 93 >>>> ++++++++++++++++++++++++----------------- >>>> 1 file changed, 54 insertions(+), 39 deletions(-) >>>> >>>> diff --git a/src/tools/sssctl/sssctl_cache.c >>>> b/src/tools/sssctl/sssctl_cache.c >>>> index >>>> >>>> e23bb89db95217e66a441b7e4d6d32e668486cc8..3318c51243c2b39feabf798091bbd5f370c2e053 >>>> 100644 >>>> --- a/src/tools/sssctl/sssctl_cache.c >>>> +++ b/src/tools/sssctl/sssctl_cache.c >>>> @@ -285,32 +285,21 @@ done: >>>> return ret; >>>> } >>>> >>>> -static errno_t sssctl_find_object(TALLOC_CTX *mem_ctx, >>>> - struct sss_domain_info *domains, >>>> - struct sss_domain_info *domain, >>>> - sssctl_basedn_fn basedn_fn, >>>> - enum cache_object obj_type, >>>> - const char *attr_name, >>>> - const char *attr_value, >>>> - const char **attrs, >>>> - struct sysdb_attrs **_entry, >>>> - struct sss_domain_info **_dom) >>>> +static const char *sssctl_create_filter(TALLOC_CTX *mem_ctx, >>>> + struct sss_domain_info *dom, >>>> + enum cache_object obj_type, >>>> + const char *attr_name, >>>> + const char *attr_value) >>>> { >>>> - TALLOC_CTX *tmp_ctx; >>>> - struct sss_domain_info *dom; >>>> - struct sysdb_attrs *entry; >>>> - struct ldb_dn *base_dn; >>>> - bool fqn_provided; >>>> - bool qualify_attr = false; >>>> - char *filter; >>>> - errno_t ret; >>>> const char *class; >>>> + const char *filter; >>>> char *filter_value; >>>> + bool qualify_attr = false; >>>> >>>> - if (strcmp(attr_name, SYSDB_NAME) == 0 && >>>> - (obj_type == CACHED_USER || >>>> - obj_type == CACHED_GROUP)) { >>>> - qualify_attr = true; >>>> + if (strcmp(attr_name, SYSDB_NAME) == 0) { >>>> + if (obj_type == CACHED_USER || obj_type == CACHED_GROUP) { >>>> + qualify_attr = true; >>>> + } >>>> >>>> This change seems unrelated to this patch. >>>> I understand someone can argue it improves readability, but then >>>> someone can argue the opposite ... >>> >>> >>> >>> And on which side are you? :-) >> >> >> My personal preference: it's cleaner in the way you proposed. >> But I really would prefer it coming in a follow up "cleaning up patch" >> instead of sneaked into this one :-) >> >> And I noticed you didn't change it in the v2 :-) >> >>> >>> >>>> >>>> } >>>> >>>> switch (obj_type) { >>>> @@ -326,9 +315,49 @@ static errno_t sssctl_find_object(TALLOC_CTX >>>> *mem_ctx, >>>> default: >>>> DEBUG(SSSDBG_FATAL_FAILURE, >>>> "sssctl doesn't handle this object type (type=%d)\n", >>>> obj_type); >>>> - return EINVAL; >>>> + return NULL; >>>> } >>>> >>>> + if (qualify_attr) { >>>> + filter_value = sss_create_internal_fqname(NULL, attr_value, >>>> dom->name); >>>> + } else { >>>> + filter_value = talloc_strdup(NULL, attr_value); >>>> + } >>>> + if (filter_value == NULL) { >>>> + DEBUG(SSSDBG_CRIT_FAILURE, "Out of memory!\n"); >>>> + return NULL; >>>> + } >>>> + >>>> + filter = talloc_asprintf(mem_ctx, "(&(objectClass=%s)(%s=%s))", >>>> + class, attr_name, filter_value); >>>> + talloc_free(filter_value); >>>> + if (filter == NULL) { >>>> + DEBUG(SSSDBG_CRIT_FAILURE, "talloc_asprintf() failed\n"); >>>> + return NULL; >>>> + } >>>> >>>> I wouldn't do this check/print this error message here, as a similar >>>> check is already done by the caller. >>>> >>>> + >>>> + return filter; >>>> +} >>>> + >>>> +static errno_t sssctl_find_object(TALLOC_CTX *mem_ctx, >>>> + struct sss_domain_info *domains, >>>> + struct sss_domain_info *domain, >>>> + sssctl_basedn_fn basedn_fn, >>>> + enum cache_object obj_type, >>>> + const char *attr_name, >>>> + const char *attr_value, >>>> + const char **attrs, >>>> + struct sysdb_attrs **_entry, >>>> + struct sss_domain_info **_dom) >>>> +{ >>>> + TALLOC_CTX *tmp_ctx; >>>> + struct sss_domain_info *dom; >>>> + struct sysdb_attrs *entry; >>>> + struct ldb_dn *base_dn; >>>> + bool fqn_provided; >>>> + const char *filter; >>>> + errno_t ret; >>>> + >>>> tmp_ctx = talloc_new(NULL); >>>> if (tmp_ctx == NULL) { >>>> DEBUG(SSSDBG_CRIT_FAILURE, "talloc_new() failed\n"); >>>> @@ -349,23 +378,9 @@ static errno_t sssctl_find_object(TALLOC_CTX >>>> *mem_ctx, >>>> goto done; >>>> } >>>> >>>> - if (qualify_attr) { >>>> - filter_value = sss_create_internal_fqname(tmp_ctx, >>>> - attr_value, >>>> - dom->name); >>>> - } else { >>>> - filter_value = talloc_strdup(tmp_ctx, attr_value); >>>> - } >>>> - if (filter_value == NULL) { >>>> - ret = ENOMEM; >>>> - goto done; >>>> - } >>>> - >>>> - filter = talloc_asprintf(tmp_ctx, "(&(objectClass=%s)(%s=%s))", >>>> - class, attr_name, filter_value); >>>> - talloc_free(filter_value); >>>> + filter = sssctl_create_filter(tmp_ctx, dom, obj_type, >>>> + attr_name, attr_value); >>>> if (filter == NULL) { >>>> - DEBUG(SSSDBG_CRIT_FAILURE, "talloc_asprintf() failed\n"); >>>> >>>> I'd keep the debug message here, but with a different message, not >>>> sure what to use though :-\ >>>> >>>> ret = ENOMEM; >>>> goto done; >>>> } >>>> >>> >>> New patch is attached. >> >> >> >> v2 looks good, apart from the comment about the if sentence that is still >> valid. > > > Ok, I split it into two patches.
Thanks a lot! Acked-by: Fabiano Fidêncio <[email protected]> Best Regards, -- Fabiano Fidêncio _______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/admin/lists/[email protected]
