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

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


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







More information about the sssd-devel mailing list