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