[SSSD] [PATCH] failover: Protect against empty host names

Simo Sorce simo at redhat.com
Tue Nov 27 16:57:04 UTC 2012


On Tue, 2012-11-27 at 11:49 -0500, Stephen Gallagher wrote:
> On Tue 27 Nov 2012 09:33:00 AM EST, Simo Sorce wrote:
> > On Tue, 2012-11-27 at 15:18 +0100, Pavel Březina wrote:
> >> Can you use "fail" pattern instead of calling the former each time
> >> allocation fails?
> >>
> >> Otherwise it looks good. I like the function.
> >>
> > I would like to vouch for always using 'done:' as a label and not other
> > arbitrary words.
> >
> > Whether it is a failure or not should be determined by testing the error
> > value. This way we always use the same pattern, which helps in reading
> > the code.
> 
> 
> There are three labels that we've used fairly consistently in the code 
> in the past:
> 
> done:
>     Used when the exit can be shared between success and failure modes, 
> possibly with some minor conditionals.
> 
> fail:
>     Used when some part of the exit processing requires 
> overly-complicated differences from the success case. I think 99% of 
> these could be merged to done: cases (and should be, where 
> appropriate), but I suspect there are a few situations where this may 
> need to remain. It should be the exception, though.
> 
> immediate:
>     Used in tevent_req routines where some part of the first phase of 
> the request (the one that initially creates the request object) fails 
> after the object is created. In this case, we need to return the object 
> and call tevent_req_immediate(), which I think should definitely remain 
> a separate label from "done:" to indicate to future maintainers that 
> exiting via this label is cutting directly to the completion callback.

I think these things should be best left to a comment in the code,
rather than labels we use for exceptions. Because those labels become
inconsistent quickly if you have to change a long function that used
done but now you would like it to return immediately for one case.

I particularly do not want to see these cases turn into having

...

done:
   ... some stuff here ...
   return foo;
immediated:
   return tevent_req_immediate();
}

I think double labels are quite nasty.

I would prefer:

done:
   if (immediate) {
       return tevent_req_immediate()
   }
   ... some stuff here ...
   return foo;
}

And just use one lable and make it always be done.

Same for fail, I came to realize it is really annoying to have to rename
a bunch of labels because after changing the code you do not have just
fail conditions but either fail or done.

If we use only 'done' then we leave fail conditions to an error code
test (forces us to use proper error codes too) and immdiate cases to an
explicit condition enforced by a boolean.

I know this is all personal preference, just laying out mine, if people
feel strongly for the current mutli-label system please speak up.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York




More information about the sssd-devel mailing list