On (16/08/16 13:30), Pavel Březina wrote:
On 08/16/2016 12:55 PM, Lukas Slebodnik wrote:
On (16/08/16 11:32), Pavel Březina wrote:
On 08/15/2016 01:21 PM, Lukas Slebodnik wrote:
On (15/08/16 11:49), Pavel Březina wrote:
On 08/15/2016 11:21 AM, Lukas Slebodnik wrote:
> > +void sbus_request_reply_error(struct sbus_request *sbus_req, > + const char *error_name, > + const char *fmt, > + ...); > + It would be good to check format string at compile time ; add SSS_ATTRIBUTE_PRINTF
Done.
> DBusError *sbus_error_new(TALLOC_CTX *mem_ctx, > - const char *dbus_err_name, > + const char *dbus_error_name, > const char *fmt, > ...) It would be good to change the name also in header file.
Done.
> + sbus_request_fail_and_finish(sbus_req, error); Is there a special reason why return code of this function is ignored? If yes then the line deserve a comment.
Because the return code is useless and the function should be void. Feel free to file a ticket/patch.
> From bca6975c580cbc16957dbb72691dbd16e1b66e7b Mon Sep 17 00:00:00 2001 > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com > Date: Thu, 14 Jul 2016 10:49:37 +0200 > Subject: [PATCH 7/7] sifp: fix coverity warning > > sssd-1.14.1/src/lib/sifp/sss_sifp_dbus.c:51: check_return: Calling "dbus_message_append_args_valist" without checking return value (as is done elsewhere 4 out of 5 times). > --- > src/lib/sifp/sss_sifp_dbus.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/src/lib/sifp/sss_sifp_dbus.c b/src/lib/sifp/sss_sifp_dbus.c > index 7c72c52f0d226ccdfaf7b8ffaed7776647a7771c..2906c5ac383c412231127f6ffa8081d47eb2bced 100644 > --- a/src/lib/sifp/sss_sifp_dbus.c > +++ b/src/lib/sifp/sss_sifp_dbus.c > @@ -36,6 +36,7 @@ static sss_sifp_error sss_sifp_ifp_call(sss_sifp_ctx *ctx, > { > DBusMessage *msg = NULL; > sss_sifp_error ret; > + dbus_bool_t bret; > > if (object_path == NULL || interface == NULL || method == NULL) { > return SSS_SIFP_INVALID_ARGUMENT; > @@ -48,7 +49,11 @@ static sss_sifp_error sss_sifp_ifp_call(sss_sifp_ctx *ctx, > } > > if (first_arg_type != DBUS_TYPE_INVALID) { > - dbus_message_append_args_valist(msg, first_arg_type, ap); > + bret = dbus_message_append_args_valist(msg, first_arg_type, ap); > + if (!bret) { > + ret = SSS_SIFP_IO_ERROR; > + goto done; > + } > } > Is this coverity warning also in master?
If answer is no then It would be better to squash it to the patch which introduced the warning. Very likely sss_sifp: make it compatible with latest version of the infopipe
I don't know, feel free to squash it.
- return error;
+}
+void sbus_request_reply_error(struct sbus_request *sbus_req,
const char *error_name,
const char *fmt,
...)
+{
- DBusError *error;
- va_list ap;
- va_start(ap, fmt);
- error = sbus_error_new_va(sbus_req, error_name, fmt, ap);
- va_end(ap);
- if (error == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "Unable to create D-Bus error, "
"killing request!\n");
talloc_free(sbus_req);
return; }
- dbus_error_init(dberr);
- dbus_set_error_const(dberr, dbus_err_name, err_msg_dup);
- return dberr;
- sbus_request_fail_and_finish(sbus_req, error);
}
struct array_arg {
2.1.0
From 67f02e1d32b346181c9ac971ea963c9af56aa674 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Mon, 27 Jun 2016 13:56:13 +0200 Subject: [PATCH 6/7] sssctl: print active server and server list
src/providers/data_provider/dp_iface.c | 4 +- src/providers/data_provider/dp_iface.h | 8 + src/providers/data_provider/dp_iface.xml | 8 + src/providers/data_provider/dp_iface_failover.c | 286 ++++++++++++++++++++++- src/providers/data_provider/dp_iface_generated.c | 52 +++++ src/providers/data_provider/dp_iface_generated.h | 10 + src/providers/fail_over.c | 42 ++++ src/providers/fail_over.h | 4 + src/responder/ifp/ifp_domains.c | 46 ++++ src/responder/ifp/ifp_domains.h | 8 + src/responder/ifp/ifp_iface.c | 4 +- src/responder/ifp/ifp_iface.xml | 10 + src/responder/ifp/ifp_iface_generated.c | 52 +++++ src/responder/ifp/ifp_iface_generated.h | 10 + src/tools/sssctl/sssctl_domains.c | 182 ++++++++++++++- 15 files changed, 708 insertions(+), 18 deletions(-)
//snip
errno_t dp_failover_list_services(struct sbus_request *sbus_req, void *dp_cli, const char *domname) {
struct sss_domain_info *domain; struct be_ctx *be_ctx; struct be_svc_data *svc; const char **services; int num_services;
errno_t ret;
bool ad_found = false;
bool ipa_found = false;
be_ctx = dp_client_be(dp_cli);
if (SBUS_IS_STRING_EMPTY(domname)) {
domain = be_ctx->domain;
} else {
domain = find_domain_by_name(be_ctx->domain, domname, false);
if (domain == NULL) {
sbus_request_reply_error(sbus_req, SBUS_ERROR_UNKNOWN_DOMAIN,
"Unknown domain %s", domname);
return EOK;
}
}
/**
* Returning list of failover services is currently rather difficult
* since there is only one failover context for the whole backend.
*
* The list of services for the given domain depends on whether it is
* a master domain or a subdomain and whether we are using IPA, AD or
* LDAP backend.
*
* For LDAP we just return everything we have.
* For AD master domain we return AD, AD_GC.
* For AD subdomain we return subdomain.com, AD_GC.
* For IPA in client mode we return IPA.
* For IPA in server mode we return IPA for master domain and
* subdomain.com, gc_subdomain.com for subdomain.
*
* We also return everything else for all cases if any other service
* such as kerberos is configured separately.
*/
/* Allocate enough space. */ num_services = 0; DLIST_FOR_EACH(svc, be_ctx->be_fo->svcs) { num_services++;
if (strcasecmp(svc->name, "AD") == 0) {
ad_found = true;
} else if (strcasecmp(svc->name, "IPA") == 0) {
ipa_found = true;
} } services = talloc_zero_array(sbus_req, const char *, num_services);
@@ -50,17 +232,103 @@ errno_t dp_failover_list_services(struct sbus_request *sbus_req, return ENOMEM; }
- num_services = 0;
- DLIST_FOR_EACH(svc, be_ctx->be_fo->svcs) {
services[num_services] = talloc_strdup(services, svc->name);
if (services[num_services] == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "talloc_strdup() failed\n");
talloc_free(services);
return ENOMEM;
}
num_services++;
- /* Fill the list. */
- ret = ERR_INTERNAL;
- if ((ad_found && ipa_found) || (!ad_found && !ipa_found)) {
/* If AD and IPA was found it is some complicated configuration,
* we return everything. Otherwise it's LDAP. */
ret = dp_failover_list_services_ldap(be_ctx, services, &num_services);
- } else if (ad_found) {
ret = dp_failover_list_services_ad(be_ctx, domain,
services, &num_services);
- } else if (ipa_found) {
ret = dp_failover_list_services_ipa(be_ctx, domain,
services, &num_services);
- }
If no branch of the above match, ret is undefined (thanks, Coverity)
I think you might overlook Jakub's comment regarding this part of code. and I do not think it's is just about coverity.
I did not overlooked it, I initialized ret prior to if's.
What about creating and bit-mask style enum enum { SIMPLE_LDAP = 0, AD_FOUND = 1, IPA_FOUND = 1 << 1, COMPLICATED_SETUP = AD_FOUND | IPA_FOUND } ^^^^^^^^^^^^ or (MIXED_IPA_AD, IPA_AD_TRUST) ,,,
I think it would make it simpler to "parse" the code for Coverity and also for mortal humas.
Nice idea. See new patches.
From d09005dea74a0b9e5d18730d4a372b5a6f4d8180 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Mon, 27 Jun 2016 13:56:13 +0200 Subject: [PATCH 6/7] sssctl: print active server and server list
src/providers/data_provider/dp_iface.c | 4 +- src/providers/data_provider/dp_iface.h | 8 + src/providers/data_provider/dp_iface.xml | 8 + src/providers/data_provider/dp_iface_failover.c | 294 ++++++++++++++++++++++- src/providers/data_provider/dp_iface_generated.c | 52 ++++ src/providers/data_provider/dp_iface_generated.h | 10 + src/providers/fail_over.c | 42 ++++ src/providers/fail_over.h | 4 + src/responder/ifp/ifp_domains.c | 46 ++++ src/responder/ifp/ifp_domains.h | 8 + src/responder/ifp/ifp_iface.c | 4 +- src/responder/ifp/ifp_iface.xml | 10 + src/responder/ifp/ifp_iface_generated.c | 52 ++++ src/responder/ifp/ifp_iface_generated.h | 10 + src/tools/sssctl/sssctl_domains.c | 182 +++++++++++++- 15 files changed, 716 insertions(+), 18 deletions(-)
diff --git a/src/providers/data_provider/dp_iface_failover.c b/src/providers/data_provider/dp_iface_failover.c index 038791088eeab7e9c5923996db77d2a107ff067d..5016b4afbb66943855114576813fc3e69fd230b9 100644 --- a/src/providers/data_provider/dp_iface_failover.c +++ b/src/providers/data_provider/dp_iface_failover.c
//snip
+static errno_t +dp_failover_list_services_ad(struct be_ctx *be_ctx,
struct sss_domain_info *domain,
const char **services,
int *_count)
+{
- char *fo_svc_name = NULL;
- struct be_svc_data *svc;
- errno_t ret;
- int count;
- fo_svc_name = talloc_asprintf(services, "sd_%s", domain->name);
- if (fo_svc_name == NULL) {
ret = ENOMEM;
goto done;
- }
- count = 0;
- DLIST_FOR_EACH(svc, be_ctx->be_fo->svcs) {
/* Drop each sd_gc_* since this service is not used with AD at all,
* we only connect to AD_GC for global catalog. */
if (strncasecmp(svc->name, "sd_gc_", strlen("sd_gc_")) == 0) {
continue;
}
/* Drop all subdomain services for different domain. */
if (strncasecmp(svc->name, "sd_", strlen("sd_")) == 0) {
if (!IS_SUBDOMAIN(domain)) {
continue;
}
if (strcasecmp(svc->name, fo_svc_name) != 0) {
continue;
}
}
if (IS_SUBDOMAIN(domain)) {
/* Drop AD since we connect to subdomain.com for LDAP. */
if (strcasecmp(svc->name, "AD") == 0) {
continue;
}
}
services[count] = talloc_strdup(services, svc->name);
if (services[count] == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "talloc_strdup() failed\n");
ret = ENOMEM;
goto done;
}
count++;
- }
- *_count = count;
- ret = EOK;
+done:
- talloc_free(fo_svc_name);
- return ret;
+}
+static errno_t +dp_failover_list_services_ipa(struct be_ctx *be_ctx,
struct sss_domain_info *domain,
const char **services,
int *_count)
+{
- struct be_svc_data *svc;
- char *fo_svc_name = NULL;
- char *fo_gc_name = NULL;
- errno_t ret;
- int count;
- fo_svc_name = talloc_asprintf(services, "sd_%s", domain->name);
- if (fo_svc_name == NULL) {
ret = ENOMEM;
goto done;
- }
- fo_gc_name = talloc_asprintf(services, "sd_gc_%s", domain->name);
- if (fo_gc_name == NULL) {
ret = ENOMEM;
goto done;
- }
- count = 0;
- DLIST_FOR_EACH(svc, be_ctx->be_fo->svcs) {
/* Drop all subdomain services for different domain. */
if (strncasecmp(svc->name, "sd_", strlen("sd_")) == 0) {
if (!IS_SUBDOMAIN(domain)) {
continue;
}
if (strcasecmp(svc->name, fo_svc_name) != 0
&& strcasecmp(svc->name, fo_gc_name) != 0) {
continue;
}
}
services[count] = talloc_strdup(services, svc->name);
if (services[count] == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "talloc_strdup() failed\n");
return ENOMEM;
}
count++;
- }
- *_count = count;
- ret = EOK;
+done:
- talloc_free(fo_svc_name);
- talloc_free(fo_gc_name);
- return ret;
+}
+enum dp_fo_svc_type {
- DP_FO_SVC_LDAP = 0,
- DP_FO_SVC_AD = 1,
- DP_FO_SVC_IPA = 1 << 1,
- DP_FO_SVC_MIXED = DP_FO_SVC_AD | DP_FO_SVC_IPA
+};
errno_t dp_failover_list_services(struct sbus_request *sbus_req, void *dp_cli, const char *domname) {
enum dp_fo_svc_type svc_type = DP_FO_SVC_LDAP;
struct sss_domain_info *domain; struct be_ctx *be_ctx; struct be_svc_data *svc; const char **services; int num_services;
errno_t ret;
be_ctx = dp_client_be(dp_cli);
if (SBUS_IS_STRING_EMPTY(domname)) {
domain = be_ctx->domain;
} else {
domain = find_domain_by_name(be_ctx->domain, domname, false);
if (domain == NULL) {
sbus_request_reply_error(sbus_req, SBUS_ERROR_UNKNOWN_DOMAIN,
"Unknown domain %s", domname);
return EOK;
}
}
/**
* Returning list of failover services is currently rather difficult
* since there is only one failover context for the whole backend.
*
* The list of services for the given domain depends on whether it is
* a master domain or a subdomain and whether we are using IPA, AD or
* LDAP backend.
*
* For LDAP we just return everything we have.
* For AD master domain we return AD, AD_GC.
* For AD subdomain we return subdomain.com, AD_GC.
* For IPA in client mode we return IPA.
* For IPA in server mode we return IPA for master domain and
* subdomain.com, gc_subdomain.com for subdomain.
*
* We also return everything else for all cases if any other service
* such as kerberos is configured separately.
*/
/* Allocate enough space. */ num_services = 0; DLIST_FOR_EACH(svc, be_ctx->be_fo->svcs) { num_services++;
if (strcasecmp(svc->name, "AD") == 0) {
svc_type |= DP_FO_SVC_AD;
} else if (strcasecmp(svc->name, "IPA") == 0) {
svc_type |= DP_FO_SVC_IPA;
}
}
services = talloc_zero_array(sbus_req, const char *, num_services);
@@ -50,17 +238,105 @@ errno_t dp_failover_list_services(struct sbus_request *sbus_req, return ENOMEM; }
- num_services = 0;
- DLIST_FOR_EACH(svc, be_ctx->be_fo->svcs) {
services[num_services] = talloc_strdup(services, svc->name);
if (services[num_services] == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "talloc_strdup() failed\n");
talloc_free(services);
return ENOMEM;
}
num_services++;
- /* Fill the list. */
- switch (svc_type) {
- case DP_FO_SVC_LDAP:
- case DP_FO_SVC_MIXED:
ret = dp_failover_list_services_ldap(be_ctx, services, &num_services);
break;
- case DP_FO_SVC_AD:
ret = dp_failover_list_services_ad(be_ctx, domain,
services, &num_services);
break;
- case DP_FO_SVC_IPA:
ret = dp_failover_list_services_ipa(be_ctx, domain,
services, &num_services);
break;
- }
- if (ret != EOK) {
DEBUG(SSSDBG_CRIT_FAILURE, "Unable to create service list [%d]: %s\n",
ret, sss_strerror(ret));
talloc_free(services);
}return ret;
coverity is happy with this version but gcc-6.1 (at least) with -O2 does not like it.
src/providers/data_provider/dp_iface_failover.c: In function ‘dp_failover_list_services’: src/providers/data_provider/dp_iface_failover.c:257:8: error: ‘ret’ may be used uninitialized in this function [-Werror=maybe-uninitialized] if (ret != EOK) { ^
Yes, it's false possitive but we do not have any warning maybe-uninitialized in master. And AS you can see I compile with Werror :-)
Thanks. Fixed.
ACK
http://sssd-ci.duckdns.org/logs/job/51/58/summary.html
LS