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.
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.
LS
_______________________________________________
sssd-devel mailing list
[email protected]
https://lists.fedorahosted.org/admin/lists/[email protected]
