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(a)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(a)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(a)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.