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?