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?= pbrezina@redhat.com > > > > > 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?= pbrezina@redhat.com > > > > > 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?= pbrezina@redhat.com > > > > > 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?= pbrezina@redhat.com > > > > > 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?= pbrezina@redhat.com > > > > > 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?= pbrezina@redhat.com > > > > > 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?= pbrezina@redhat.com > > > > > 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?= pbrezina@redhat.com 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?= pbrezina@redhat.com 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?= pbrezina@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
LS