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.
Best Regards,
--
Fabiano Fidêncio
_______________________________________________
sssd-devel mailing list
[email protected]
https://lists.fedorahosted.org/admin/lists/[email protected]
