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?= <pbrez...@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?= <pbrez...@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?= <pbrez...@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
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
