[SSSD] [PATCHES] fix SRV expansion

Lukas Slebodnik lslebodn at redhat.com
Wed Jun 19 12:20:35 UTC 2013


On (19/06/13 13:59), Pavel Březina wrote:
>On 06/19/2013 01:43 PM, Jakub Hrozek wrote:
>>On Wed, Jun 19, 2013 at 10:51:32AM +0200, Lukas Slebodnik wrote:
>>>On (18/06/13 13:51), Pavel Březina wrote:
>>>>https://fedorahosted.org/sssd/ticket/1947
>>>>
>>>>A fast explanation how _srv_ expansion works. _srv_ is inserted into
>>>>server list as so called meta server. Let us consider following
>>>>configuration:
>>>>
>>>>*Setup*
>>>>ipa_server = _srv_, ipa.pb
>>>>server list will contain: meta -> ipa.pb
>>>>
>>>>*Expansion*
>>>>meta -> ipa.pb:389 -> ipa.pb ; remove meta
>>>>ipa.pb:389 -> ipa.pb ; meta
>>>>
>>>>*Collapse*
>>>>remove ipa.pb:389 ; insert meta
>>>>meta -> ipa.pb
>>>>
>>>>The main problem is that expanded SRV servers are marked as NEUTRAL
>>>>during online check, but they don't collapse back into a meta server.
>>>>
>>>>This will trigger another SRV expansion, leaving the old server in
>>>>the list and trying to add the servers again. This is present in both
>>>>master and 1.9 (and probably older versions), although the result is
>>>>slightly different.
>>>>
>>>>In master, we don't insert a server into server list if it is already
>>>>present. Because state->meta is orphaned from the previous SRV
>>>>expansion, state->meta->next is NULL and SSSD crashes later.
>>>>
>>>I can confirm, tah patches fix ticket 1947 (crash).
>>>
>>>>In 1.9, we simply insert duplicate servers. Those servers are
>>>>inserted after orphaned state->meta, state->meta is orphaned again,
>>>>leaving those servers globally unreachable. However, it seemingly
>>>>does not affect the fail over. You just run into d25e7c65.
>>>>
>>>>Here are four patches for master, and two patches for 1.9.
>>>
>>>I have just a question about code convention.
>>>I thought, that name of function parameter starting with "_" means
>>>output variable, but in function collapse_srv_lookup it is used as
>>>a in/out variable.
>>>
>>>If some function is called with NULL in place of output parameter, it means
>>>that you do not want store output to this parameter. But function
>>>collapse_srv_lookup could not be called with NULL
>>>
>>>>static struct fo_server *
>>>>-collapse_srv_lookup(struct fo_server *server)
>>>>+collapse_srv_lookup(struct fo_server **_server)
>>>>{
>>>>-    struct fo_server *tmp, *meta;
>>>>+    server = *_server;
>>>                 ^^^^^^^
>>>         This is reason, why function could not be called with NULL.
>>>
>>>Do we have any code conventions for in/out variable?
>>
>>I don't think we do. Does the logic of collapse_srv_lookup mandate that
>>the input and output are always the same? If not, can we simply add an
>>input parameter that can't be NULL?
>
>It's not real in/out parameter. The thing is that
>collapse_srv_lookup(server) frees the server, so I wanted to make
>sure that it is set to NULL.
>
>Old call:
>meta = collapse_srv_lookup(server);
>
>Now:
>meta = collapse_srv_lookup(&server); server is set to NULL

It is in/out parameter, because value of server is(may be) changed after
function call. (it is not important, that value will be NULL)

LS



More information about the sssd-devel mailing list