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

Lukas Slebodnik lslebodn at redhat.com
Mon Jun 10 12:22:41 UTC 2013


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

NACK
sssd_be crashed with your patch.

state->meta->next is still NULL in function resolve_srv_done.

LS



More information about the sssd-devel mailing list