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