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