On (16/08/16 14:21), Lukas Slebodnik wrote: >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?= <[email protected]> >>> > > > > > 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?= <[email protected]> >>> > > > 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?= <[email protected]> >>> > 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 > master: * de5160e354c02020f0593c7cabdb811107d5d8e2 * bd4c2ed5aec7f57ea04500f0e43f151eedfdde45 * 778f241e78241b0d6b8734148175f8dee804f494 * 9b74009c1260e6f3b1031a6ae110bf1d957cba81 * 439e08cdc5c83b3e5835cb0435983f1da2ffbaf1 * a06e23c0bcf0c8669a29b801876aca8aac422931 * a40d9cc11d17d9c3c22a0462cd8c419d1e79ffb8
LS _______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/admin/lists/[email protected]
