[SSSD] [PATCH] Use meta server if there isn't another server after meta.

Pavel Březina pbrezina at redhat.com
Mon Jun 10 08:54:03 UTC 2013


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 at 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 at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-failover-do-not-return-invalid-pointer-when-server-i.patch
Type: text/x-patch
Size: 1574 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20130610/421775df/attachment.bin>


More information about the sssd-devel mailing list