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.