[SSSD] [PATCHES] fix SRV expansion

Jakub Hrozek jhrozek at redhat.com
Wed Jun 19 19:13:43 UTC 2013


On Wed, Jun 19, 2013 at 02:20:35PM +0200, Lukas Slebodnik wrote:
> 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)
> 

Sorry, that's details. I think that it's important that the function
synopsis makes it clear that the parameter can be modified.



More information about the sssd-devel mailing list