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?= <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 :-)
>>
>
>Thanks. Fixed.
>
ACK
http://sssd-ci.duckdns.org/logs/job/51/58/summary.html
LS
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
