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?