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