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.
