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.
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.
LS