[SSSD] Re: [PATCHES] sssctl: print active server and server list

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 :-)

LS