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&gt; >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

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&gt; >>> 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 _______________________________________________ sssd-devel mailing list sssd-devel(a)lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

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&gt; >>>>> 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