On (12/08/16 11:40), Pavel Březina wrote: >On 08/12/2016 11:37 AM, Jakub Hrozek wrote: >> On Thu, Aug 11, 2016 at 11:49:07AM +0200, Pavel Březina wrote: >> > On 08/09/2016 11:21 AM, Jakub Hrozek wrote: >> > > On Fri, Aug 05, 2016 at 12:02:19PM +0200, Pavel Březina wrote: >> > > > On 07/25/2016 12:55 PM, Pavel Březina wrote: >> > > > > On 07/20/2016 03:03 PM, Jakub Hrozek wrote: >> > > > > > On Wed, Jul 20, 2016 at 03:00:14PM +0200, Jakub Hrozek wrote: >> > > > > > > On Tue, Jul 19, 2016 at 12:20:16PM +0200, Pavel Březina wrote: >> > > > > > > > On 07/18/2016 03:23 PM, Jakub Hrozek wrote: >> > > > > > > > > On Thu, Jul 14, 2016 at 11:39:40AM +0200, Pavel Březina >> > > > > > > > > wrote: >> > > > > > > > > > From 6af064012844f3e61b9fa5b502bebe44e4620ffd Mon Sep >> > > > > > > > > > 17 >> > > > > > > > > > 00:00:00 2001 >> > > > > > > > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= >> > > > > > > > > > <[email protected]> >> > > > > > > > > > Date: Tue, 28 Jun 2016 11:40:16 +0200 >> > > > > > > > > > Subject: [PATCH 1/7] rdp: add ability to forward reply to >> > > > > > > > > > the >> > > > > > > > > > client request >> > > > > > > > > >> > > > > > > > > LGTM, but I didn't test yet. >> > > > > > > > > >> > > > > > > > > > From 2ad0b97f9e435ac5f57caf37479304c16d0424c9 Mon Sep >> > > > > > > > > > 17 >> > > > > > > > > > 00:00:00 2001 >> > > > > > > > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= >> > > > > > > > > > <[email protected]> >> > > > > > > > > > Date: Tue, 28 Jun 2016 15:29:39 +0200 >> > > > > > > > > > Subject: [PATCH 2/7] sbus: add sbus_request_reply_error() >> > > > > > > > > >> > > > > > > > > OK, but what is the plan on the other calls? I wouldn't like >> > > > > > > > > us to end >> > > > > > > > > up with two competing implementations of the same (again).. >> > > > > > > > >> > > > > > > > It is sometimes desirable to pass DBusError as an argument but >> > > > > > > > in >> > > > > > > > much more >> > > > > > > > cases it is simpler to just use this call. The plan is to >> > > > > > > > change the >> > > > > > > > calls >> > > > > > > > where it is desirable, I'll send separate patch later for >> > > > > > > > this. Here I >> > > > > > > > changed it only for sssctl related code. >> > > > > > > > >> > > > > > > > > > From 0f508095599b3e04573a21a600b4f42e172cc304 Mon Sep >> > > > > > > > > > 17 >> > > > > > > > > > 00:00:00 2001 >> > > > > > > > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= >> > > > > > > > > > <[email protected]> >> > > > > > > > > > Date: Wed, 29 Jun 2016 12:35:59 +0200 >> > > > > > > > > > Subject: [PATCH 3/7] sbus: add utility function to simplify >> > > > > > > > > > message and reply >> > > > > > > > > > handling >> > > > > > > > > > >> > > > > > > > > > This patch adds the ability to hook DBusMessage to a >> > > > > > > > > > talloc context >> > > > > > > > > > to remove the need of calling dbus_message_unref(). It >> > > > > > > > > > also provides >> > > > > > > > > > an automatical way to detect error in a reply so the >> > > > > > > > > > caller does >> > > > > > > > > > not need to parse it manually and the whole code around >> > > > > > > > > > DBusError >> > > > > > > > > > can be avoided. >> > > > > > > > > >> > > > > > > > > One code readability nitpick.. >> > > > > > > > > >> > > > > > > > > > +errno_t sbus_check_reply(DBusMessage *reply) >> > > > > > > > > > +{ >> > > > > > > > > > + dbus_bool_t bret; >> > > > > > > > > > + DBusError error; >> > > > > > > > > > + errno_t ret; >> > > > > > > > > > + >> > > > > > > > > > + dbus_error_init(&error); >> > > > > > > > > > + >> > > > > > > > > > + switch (dbus_message_get_type(reply)) { >> > > > > > > > > > + case DBUS_MESSAGE_TYPE_METHOD_RETURN: >> > > > > > > > > > + ret = EOK; >> > > > > > > > > > + goto done; >> > > > > > > > > >> > > > > > > > > Can you use break in the non-failure cases? I would say this >> > > > > > > > > would be >> > > > > > > > > more readable.. >> > > > > > > > >> > > > > > > > Done. >> > > > > > > > >> > > > > > > > > > + >> > > > > > > > > > + case DBUS_MESSAGE_TYPE_ERROR: >> > > > > > > > > > + bret = dbus_set_error_from_message(&error, reply); >> > > > > > > > > > + if (bret == false) { >> > > > > > > > > > + DEBUG(SSSDBG_CRIT_FAILURE, "Unable to read >> > > > > > > > > > error from >> > > > > > > > > > message\n"); >> > > > > > > > > > + ret = EIO; >> > > > > > > > > > + goto done; >> > > > > > > > > > + } >> > > > > > > > > > + >> > > > > > > > > > + DEBUG(SSSDBG_CRIT_FAILURE, "D-Bus error [%s]: >> > > > > > > > > > %s\n", >> > > > > > > > > > + error.name, (error.message == NULL ? >> > > > > > > > > > "(null)" : >> > > > > > > > > > error.message)); >> > > > > > > > > > + ret = sbus_error_to_errno(&error); >> > > > > > > > > > + goto done; >> > > > > > > > > > + default: >> > > > > > > > > > + DEBUG(SSSDBG_CRIT_FAILURE, "Unexpected D-Bus >> > > > > > > > > > message >> > > > > > > > > > type?\n"); >> > > > > > > > > > + ret = ERR_INTERNAL; >> > > > > > > > > > + goto done; >> > > > > > > > > > + } >> > > > > > > > > > + >> > > > > > > > > > +done: >> > > > > > > > > > + dbus_error_free(&error); >> > > > > > > > > > + >> > > > > > > > > > + return ret; >> > > > > > > > > > +} >> > > > > > > > > >> > > > > > > > > The rest looks good to me. >> > > > > > > > > >> > > > > > > > > > From 93b4f2e9077f1959d563ba8e4d3f41a9764cc8c6 Mon Sep >> > > > > > > > > > 17 >> > > > > > > > > > 00:00:00 2001 >> > > > > > > > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= >> > > > > > > > > > <[email protected]> >> > > > > > > > > > Date: Wed, 29 Jun 2016 14:03:38 +0200 >> > > > > > > > > > Subject: [PATCH 4/7] sssctl: use talloc with sifp >> > > > > > > > > >> > > > > > > > > LGTM >> > > > > > > > > >> > > > > > > > > > From 33f5fd436a0fc3d5d1eac970b66c174bb2da6189 Mon Sep >> > > > > > > > > > 17 >> > > > > > > > > > 00:00:00 2001 >> > > > > > > > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= >> > > > > > > > > > <[email protected]> >> > > > > > > > > > Date: Wed, 29 Jun 2016 14:58:37 +0200 >> > > > > > > > > > Subject: [PATCH 5/7] failover: mark subdomain service with >> > > > > > > > > > sd_ prefix >> > > > > > > > > >> > > > > > > > > Did you test the failover with subdomain? I wonder if we >> > > > > > > > > rely on the >> > > > > > > > > service name somewhere, but I can't remember. >> > > > > > > > >> > > > > > > > Yes. The name is generated and stored in service context. We >> > > > > > > > do not >> > > > > > > > rely on >> > > > > > > > precise name anywhere, but in these patches... it's not a nice >> > > > > > > > thing >> > > > > > > > to do >> > > > > > > > but I didn't come up with anything better. >> > > > > > > > >> > > > > > > > > But more importantly, also related to the next patch, you >> > > > > > > > > noted we >> > > > > > > > > don't >> > > > > > > > > actually use the GC service for AD subdomains, if that's true >> > > > > > > > > shouldn't >> > > > > > > > > we remove it completely? >> > > > > > > > >> > > > > > > > We use it with IPA provider. But it is doable to remove it >> > > > > > > > from AD >> > > > > > > > provider >> > > > > > > > completely, but I didn't want to touch fo service code with >> > > > > > > > those >> > > > > > > > patches. >> > > > > > > > >> > > > > > > > > > From e4c6e829a1942290d441cec618d477782bbe4c08 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] ssctl: print active server and server >> > > > > > > > > > list >> > > > > > > > > > + 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; >> > > > > > > > > >> > > > > > > > > I asked this in the previous patch, shouldn't we drop the GC >> > > > > > > > > service if >> > > > > > > > > it's not used? Also, what does "we only connect to AD_GC for >> > > > > > > > > global >> > > > > > > > > catalog." mean? >> > > > > > > > >> > > > > > > > There is service called AD_GC. We resolve this service when we >> > > > > > > > want to >> > > > > > > > connect to GC whether we search in subdomain or not when using >> > > > > > > > AD >> > > > > > > > provider. >> > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > > + } >> > > > > > > > > > + >> > > > > > > > > > + /* Drop all subdomain services for different >> > > > > > > > > > domain. */ >> > > > > > > > > > + if (strncasecmp(svc->name, "sd_", strlen("sd_")) >> > > > > > > > > > == 0) { >> > > > > > > > > > + if (!IS_SUBDOMAIN(domain)) { >> > > > > > > > > > + continue; >> > > > > > > > > >> > > > > > > > > Why would we see sd_ services for non-subdomin? >> > > > > > > > >> > > > > > > > Because the failover context is shared across all domains in >> > > > > > > > backend, so >> > > > > > > > there's always everything. >> > > > > > > > >> > > > > > > > > > + } >> > > > > > > > > > + >> > > > > > > > > > + 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; >> > > > > > > > > > + } >> > > > > > > > > > + } >> > > > > > > > > >> > > > > > > > > > From e74a27309b8a49c0f524bb9d3d66385dd1d33bc1 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 >> > > > > > > > > >> > > > > > > > > ACK >> > > > > > > > >> > > > > > > > New patches attached. >> > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > > OK, one more question and I'm not sure if failover is at fault >> > > > > > > here..I >> > > > > > > tested on an IPA server connected to an AD domain where I >> > > > > > > switched off >> > > > > > > one of the DCs. Now the domain reports like this: >> > > > > > > >> > > > > > > $ sudo sssctl domain-status subdom.win.trust.test >> > > > > > > Online status: Offline >> > > > > > > >> > > > > > > Active servers: >> > > > > > > AD Domain Controller: child.subdom.win.trust.test >> > > > > > > IPA: unidirect.ipa.test >> > > > > > > >> > > > > > > Discovered AD Domain Controller servers: >> > > > > > > - child.subdom.win.trust.test >> > > > > > > >> > > > > > > Discovered IPA servers: >> > > > > > > - unidirect.ipa.test >> > > > > > > >> > > > > > > So sssctl correctly reports the domain is offline, but still >> > > > > > > prints an >> > > > > > > active server..why? >> > > > > > >> > > > > > also, there is a warning: >> > > > > > Error: COMPILER_WARNING: >> > > > > > sssd-1.14.1/src/providers/data_provider/dp_iface_failover.c: >> > > > > > scope_hint: In function 'dp_failover_list_services' >> > > > > > sssd-1.14.1/src/providers/data_provider/dp_iface_failover.c:248:8: >> > > > > > warning: 'ret' may be used uninitialized in this function >> > > > > > [-Wmaybe-uninitialized] >> > > > > > # if (ret != EOK) { >> > > > > > # ^ >> > > > > > # 246| } >> > > > > > # 247| >> > > > > > # 248|-> if (ret != EOK) { >> > > > > > # 249| DEBUG(SSSDBG_CRIT_FAILURE, "Unable to create >> > > > > > service >> > > > > > list [%d]: %s\n", >> > > > > > # 250| ret, sss_strerror(ret)); >> > > > > >> > > > > I switched to be_fo_get_active_server_name() which I didn't see >> > > > > before. >> > > > > It seems alright now. Thanks for review. New patches are attached. >> > > > >> > > > Bump. >> > > >> > > I will review the patches today again, but can you rebase them? >> > >> > Rebased patches are attached. >> > >> >> I'm sorry, but I still can't apply these: >> $ git am *.patch >> Applying: rdp: add ability to forward reply to the client request >> Applying: sbus: add sbus_request_reply_error() >> Applying: sbus: add utility function to simplify message and reply handling >> Applying: sssctl: use talloc with sifp >> error: patch failed: src/tools/sssctl/sssctl.h:56 >> error: src/tools/sssctl/sssctl.h: patch does not apply >> Patch failed at 0004 sssctl: use talloc with sifp >> The copy of the patch that failed is found in: >> /home/remote/jhrozek/devel/sssd/.git/rebase-apply/patch >> When you have resolved this problem, run "git am --continue". >> If you prefer to skip this patch, run "git am --skip" instead. >> To restore the original branch and stop patching, run "git am --abort". > >I'm sorry, I sent wrong patches. How about now? >
Few comments/nitpicks which I found when I was looking into patches which depend on those. >From bbec20120a76c27f2ffeb3ef9a8bd6d985ace27f Mon Sep 17 00:00:00 2001 >From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <[email protected]> >Date: Tue, 28 Jun 2016 15:29:39 +0200 >Subject: [PATCH 2/7] sbus: add sbus_request_reply_error() > >This simplifies error handling in sbus requests since we avoid >creating DBusError and checking for NULL manually. It removes >few lines of code. > >This patch does not replace all calls to sbus_request_fail_and_finish >since sometimes it is desirable to create the error manualy. But >it replaces it in most recent places. >--- > src/providers/data_provider/dp_iface_backend.c | 11 ++-- > src/responder/ifp/ifp_domains.c | 12 ++--- > src/sbus/sssd_dbus.h | 5 ++ > src/sbus/sssd_dbus_request.c | 73 ++++++++++++++++++++------ > 4 files changed, 68 insertions(+), 33 deletions(-) > >diff --git a/src/sbus/sssd_dbus.h b/src/sbus/sssd_dbus.h >index >fe1c4a7e1730e088647744d9b49a68c3c71db57f..3bce46b192a18d1e4c732e7fd3488ae17b84d10c > 100644 >--- a/src/sbus/sssd_dbus.h >+++ b/src/sbus/sssd_dbus.h >@@ -357,6 +357,11 @@ int sbus_request_return_and_finish(struct sbus_request >*dbus_req, > int sbus_request_fail_and_finish(struct sbus_request *dbus_req, > const DBusError *error); > >+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 > /* > * Construct a new DBusError instance which can be consumed by functions such > * as @sbus_request_fail_and_finish(). >diff --git a/src/sbus/sssd_dbus_request.c b/src/sbus/sssd_dbus_request.c >index >f8647b5ecfb4a49d45a15733b22c6014f4bd084c..0cc9bb3db5bfbfb2eaad1fca76d6683d3ed0d0b1 > 100644 >--- a/src/sbus/sssd_dbus_request.c >+++ b/src/sbus/sssd_dbus_request.c >@@ -199,31 +199,70 @@ int sbus_request_fail_and_finish(struct sbus_request >*dbus_req, > return ret; > } > >+static DBusError *sbus_error_new_va(TALLOC_CTX *mem_ctx, >+ const char *error_name, >+ const char *fmt, >+ va_list ap) >+{ >+ DBusError *error; >+ const char *error_msg; >+ >+ error = talloc_zero(mem_ctx, DBusError); >+ if (error == NULL) { >+ return NULL; >+ } >+ >+ if (fmt != NULL) { >+ error_msg = talloc_vasprintf(error, fmt, ap); >+ if (error_msg == NULL) { >+ talloc_free(error); >+ return NULL; >+ } >+ } else { >+ error_msg = NULL; >+ } >+ >+ dbus_error_init(error); >+ dbus_set_error_const(error, error_name, error_msg); >+ >+ return error; >+} >+ > 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. > { >- DBusError *dberr; >- const char *err_msg_dup = NULL; >+ DBusError *error; > va_list ap; > >- dberr = talloc(mem_ctx, DBusError); >- if (dberr == NULL) return NULL; >- >- if (fmt) { >- va_start(ap, fmt); >- err_msg_dup = talloc_vasprintf(dberr, fmt, ap); >- va_end(ap); >- if (err_msg_dup == NULL) { >- talloc_free(dberr); >- return NULL; >- } >+ va_start(ap, fmt); >+ error = sbus_error_new_va(mem_ctx, dbus_error_name, fmt, ap); >+ va_end(ap); >+ >+ 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); Is there a special reason why return code of this function is ignored? If yes then the line deserve a comment. > } > > struct array_arg { >-- >2.1.0 > >From 76ec11aebe14d38202c1e87b960caaff005a2540 Mon Sep 17 00:00:00 2001 >From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <[email protected]> >Date: Wed, 29 Jun 2016 14:03:38 +0200 >Subject: [PATCH 4/7] sssctl: use talloc with sifp > >This way we completely move D-Bus memory management to talloc and >we reduce number of code lines needed to send and receive reply. >--- > src/tools/sssctl/sssctl.h | 14 +++++++++ > src/tools/sssctl/sssctl_domains.c | 62 ++++++++++++++++++--------------------- > src/tools/sssctl/sssctl_sifp.c | 46 +++++++++++++++++++++++++++++ > 3 files changed, 88 insertions(+), 34 deletions(-) > >diff --git a/src/tools/sssctl/sssctl_domains.c >b/src/tools/sssctl/sssctl_domains.c >index >17ad670f39dfc045ba090210ffcfa77df713c306..40962792b84eabeb2c142f158184b17180a01669 > 100644 >--- a/src/tools/sssctl/sssctl_domains.c >+++ b/src/tools/sssctl/sssctl_domains.c >@@ -74,47 +74,32 @@ errno_t sssctl_domain_list(struct sss_cmdline *cmdline, > } > > static errno_t sssctl_domain_status_online(struct sss_tool_ctx *tool_ctx, ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ argument is unused after this change. >- const char *domain_path, >- bool force_start) >+ sss_sifp_ctx *sifp, >+ const char *domain_path) > { >- sss_sifp_ctx *sifp; >- sss_sifp_error sifp_error; >- DBusMessage *reply = NULL; >- DBusMessage *msg; >+ TALLOC_CTX *tmp_ctx; >+ sss_sifp_error error; >+ DBusMessage *reply; > bool is_online; > errno_t ret; > >- if (!sssctl_start_sssd(force_start)) { >- ret = ERR_SSSD_NOT_RUNNING; >- goto done; >+ tmp_ctx = talloc_new(NULL); >+ if (tmp_ctx == NULL) { >+ DEBUG(SSSDBG_CRIT_FAILURE, "talloc_new() failed\n"); >+ return ENOMEM; > } > >- sifp_error = sssctl_sifp_init(tool_ctx, &sifp); >- if (sifp_error != SSS_SIFP_OK) { >- sssctl_sifp_error(sifp, sifp_error, "Unable to connect to the >InfoPipe"); >- ret = EFAULT; >- goto done; >- } >- >- msg = sbus_create_message(tool_ctx, SSS_SIFP_ADDRESS, domain_path, >- IFACE_IFP_DOMAINS_DOMAIN, >- IFACE_IFP_DOMAINS_DOMAIN_ISONLINE); >- if (msg == NULL) { >- DEBUG(SSSDBG_CRIT_FAILURE, "Unable to create D-Bus message\n"); >- ret = ENOMEM; >- goto done; >- } >- >- sifp_error = sss_sifp_send_message(sifp, msg, &reply); >- if (sifp_error != SSS_SIFP_OK) { >- sssctl_sifp_error(sifp, sifp_error, "Unable to get online status"); >+ error = sssctl_sifp_send(tmp_ctx, sifp, &reply, domain_path, >+ IFACE_IFP_DOMAINS_DOMAIN, >+ IFACE_IFP_DOMAINS_DOMAIN_ISONLINE); >+ if (error != SSS_SIFP_OK) { >+ sssctl_sifp_error(sifp, error, "Unable to get online status"); > ret = EIO; > goto done; > } > > ret = sbus_parse_reply(reply, DBUS_TYPE_BOOLEAN, &is_online); > if (ret != EOK) { >- fprintf(stderr, _("Unable to get information from SSSD\n")); > goto done; > } > >@@ -123,10 +108,7 @@ static errno_t sssctl_domain_status_online(struct >sss_tool_ctx *tool_ctx, > ret = EOK; > > done: >- if (reply != NULL) { >- dbus_message_unref(reply); >- } >- >+ talloc_free(tmp_ctx); > return ret; > } >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 LS _______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/admin/lists/[email protected]
