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(a)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(a)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(a)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(a)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(a)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(a)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(a)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.