6:31 a.m.
Replying with comments in-line:
From a4a2eb297e6e56e6d0d05ab3810ab87eb320ebdb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
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 ...
}
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;
}
--
2.1.0
Reviewed-by: Fabiano Fidêncio <fidencio(a)redhat.com>
--
Fabiano Fidêncio
6:37 a.m.
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?= <pbrezina(a)redhat.com>
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? :-)
}
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.
6:49 a.m.
On Tue, Jul 12, 2016 at 1:37 PM, Pavel Březina <pbrezina(a)redhat.com> 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?= <pbrezina(a)redhat.com>
> 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
3:41 a.m.
On 07/12/2016 01:49 PM, Fabiano Fidêncio wrote:
On Tue, Jul 12, 2016 at 1:37 PM, Pavel Březina
<pbrezina(a)redhat.com> 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?= <pbrezina(a)redhat.com>
>> 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.
3:44 a.m.
On Wed, Jul 13, 2016 at 10:41 AM, Pavel Březina <pbrezina(a)redhat.com> wrote:
On 07/12/2016 01:49 PM, Fabiano Fidêncio wrote:
>
> On Tue, Jul 12, 2016 at 1:37 PM, Pavel Březina <pbrezina(a)redhat.com>
> 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?= <pbrezina(a)redhat.com>
>>> 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 <fidencio(a)redhat.com>
Best Regards,
--
Fabiano Fidêncio
5:08 a.m.
New subject: [PATCH] sssctl: move filter creation to separate function
On Wed, Jul 13, 2016 at 10:44:18AM +0200, Fabiano Fidêncio wrote:
Thanks a lot!
Acked-by: Fabiano Fidêncio <fidencio(a)redhat.com>
* master:
* 3c6e15e8aa38d9dfa02a7255fad56149bdfb35a6
* aa691837a2fa2fe2e38a55d576644074e0f45bd8
2867
days inactive
2868
days old
sssd-devel@lists.fedorahosted.org
6 comments
3 participants
participants (3)
-
Fabiano Fidêncio -
Jakub Hrozek -
Pavel Březina