On (10/06/13 10:54), Pavel Březina wrote:
> On 06/10/2013 10:32 AM, Pavel Březina wrote:
>> On 06/03/2013 10:28 PM, Jakub Hrozek wrote:
>>> On Mon, Jun 03, 2013 at 03:54:34PM +0200, Lukas Slebodnik wrote:
>>>> On (03/06/13 12:50), Lukas Slebodnik wrote:
>>>>> ehlo
>>>>>
>>>>> I solved ticket #1929 and then I immediately hit another one #1947
>>>>> (BZ967004)
>>>>> It seems that reporter from second ticket was lucky and did not hit
>>>>> the first
>>>>> issue.
>>>>>
>>>>> Use meta server if there isn't another server after meta.
>>>>> The same behaviour is in "done" section, if return code is
not OK
>>>>>
>>>>>
https://fedorahosted.org/sssd/ticket/1947
>>>>>
>>>>> Patch is attached.
>>>>>
>>>>> LS
>>>>
>>>> >From f885ea412076843b3e6bf97be51a2e7ae8a0240a Mon Sep 17 00:00:00
2001
>>>>> From: Lukas Slebodnik <lslebodn(a)redhat.com>
>>>>> Date: Fri, 31 May 2013 14:18:17 +0200
>>>>> Subject: [PATCH] Use meta server if there isn't another server
after
>>>>> meta.
>>>>>
>>>>> The same behaviour is in "done" section, if return code is
not OK
>>>>>
>>>>>
https://fedorahosted.org/sssd/ticket/1947
>>>>> ---
>>>>> src/providers/fail_over.c | 2 +-
>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/src/providers/fail_over.c b/src/providers/fail_over.c
>>>>> index
>>>>>
12b6c37828b7da0e68579bbb94668c21574974f1..6fa8b19c6e14d778d5f438b1578f3a22876b1625
>>>>> 100644
>>>>> --- a/src/providers/fail_over.c
>>>>> +++ b/src/providers/fail_over.c
>>>>> @@ -1285,7 +1285,7 @@ resolve_srv_done(struct tevent_req *subreq)
>>>>> }
>>>>> }
>>>>>
>>>>> - state->out = state->meta->next;
>>>>> + state->out = (state->meta->next) ?
state->meta->next :
>>>>> state->meta;
>>>> ^^^^^^^^
>>>> This code is little bit different in sssd-1-9 branch
>>>> Somethink like this
>>>> if (srv_list) {
>>>> <snip>
>>>> state->out = srv_list
>>>>
>>>> tevent_req_done(req);
>>>> return;
>>>> } else {
>>>> ret = EIO;
>>>> goto fail;
>>>> }
>>>>
>>>> Should We return EIO? Or my solution is good enough.
>>>>
>>>> LS
>>>
>>> Sorry, I don't think this is the right fix.
>>>
>>> The simplified synopsis of the resolve_srv request is:
>>> resolve_srv_send(server_in);
>>> resolve_srv_recv(server_out);
>>>
>>> server_in is a SRV meta-server that we want to resolve, server_out is
>>> the first server in the list we have resolved.
>>>
>>> Currently, as you noted, server_out is returned even when SRV resolution
>>> fails, but I don't think that's needed, if the SRV resolution fails,
the
>>> meta-server is marked as bad inside the request. Also, the failover code
>>> uses the returned server unconditionally.
>
> If the resolution fails, server_out is the original meta server with
> error flag. Failover will then try to resolve next server.
>
>>> So one part of the fix would be to make sure the resolve_srv_* request
>>> always acts on the meta server and only return a fo_server on success.
>>> The server that we return should be the "_last_server" parameter
of
>>> fo_add_server_list().
>
> No. meta->next should be returned on success. last_server is only for
> ordering purpose. meta->next is the next server to which sssd should
> try to connect.
>
>>> The second part of the bug comes from the fact, that, according to the
>>> logs, the same server the query resolved was already present in the
>>> list. Here is the function fo_add_server_list():
>
> Patch for this is attached.
>
>>>
>>> 740 for (i = 0; i < num_servers; i++) {
>>> 741 server = create_fo_server(service, servers[i].host,
>>> servers[i].port,
>>> 742 user_data, primary);
>>> 743 if (server == NULL) {
>>> 744 talloc_free(srv_list);
>>> 745 return ENOMEM;
>>> 746 }
>>> 747
>>> 748 server->srv_data = srv_data;
>>> 749
>>> 750 ret = fo_add_server_to_list(&srv_list,
service->server_list,
>>> 751 server, service->name);
>>> 752 if (ret != EOK) {
>>>
>>> Because the server is already in the list, fo_add_server_to_list()
>>> returns
>>> EEXIST here.
>>>
>>> 753 talloc_free(server);
>>>
>>> The pointer to "server" is freed.
>>>
>>> 754 }
>>>
>>> The loop breaks because num_servers == 1
>>>
>>> 755 }
>>> 756
>>> 757 if (srv_list != NULL) {
>>> 758 DLIST_ADD_LIST_AFTER(service->server_list, after_server,
>>> 759 srv_list, struct fo_server *);
>>> 760 }
>>> 761
>>> 762 if (_last_server != NULL) {
>>> 763 *_last_server = server;
>>>
>>> Now server is assigned to _last_server, but points to freed memory..
>>>
>>> 764 }
>>>
>>> I think we should instead assign srv_list to _last_server. I'm not quite
>>> sure why we used the tail of the list, maybe Pavel will remember? But I
>>> think we should return the head of what we resolved to the caller.
>>
>> Hi,
>> the tail is used to keep the order of the servers.
>>
>> metasrv -> srv1_1 (head) -> srv1_2 (last server) -> srv2_1 -> srv2_2
>>
>> If you return head (ie srv_list) you get:
>> metasrv -> srv1_1 (last_server) -> srv2_1 -> srv2_2 -> srv1_2
>>
>>
>>
>>
>> _______________________________________________
>> sssd-devel mailing list
>> sssd-devel(a)lists.fedorahosted.org
>>
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
NACK
sssd_be crashed with your patch.
state->meta->next is still NULL in function resolve_srv_done.
LS