[illumos-Developer] Review for 89: Bug uu_list_find() leaves error unset when successfully returning NULL

[Garrett D'Amore]

While the API can be changed, its not *that* bad after looking at it.

The typical caller is going to just look for an element... and expect it
to return success.

The problem is that if the list is malformed, or really if the list
doesn't support this function, then we return NULL just like we do when
we can't find the element.

I'd almost rather have had the error case at 326 been an ASSERT(),
because clearly someone is misusing the list when it occurs.  I can't
think of a correct program that would ever return failure here.

This particular bug is really just a nit, and I think Jason's handling
of it is fine.

	- Garrett

On Thu, 2011-01-20 at 10:53 +0100, Joerg Schilling wrote:
> Eric Schrock <eric.schrock at delphix.com> wrote:
> 
> > Ugh, not the most pleasant API design, but nothing that can really be
> > changed, now.  A simple test case might be:
> >
> > 1. Create two lists, one with a NULL comparison routine, and a normal one.
> > 2. Try to search in the one with a NULL comparison routine (sets libuutil
> > error)
> > 3. Try to search for a non-existent entry in the normal one (should have a
> > zero errno)
> >
> > Otherwise looks OK to me.  This reminds me of strtol() and friends, where
> > most consumers are forced to preface it with 'errno = 0' to distinguish a
> > zero value from an invalid entry.  This seems like a better solution than
> > exporting that complexity to the user.
> 
> The idea behind this concept is to never change errno in case there was no error.
> 
> This allows to call any such function from inside other functions without being 
> forced to save and restore the previous errno. Note that there are many 
> situations where you like to return a previous "better" errno in case something 
> fails.
> 
> If you ever see a function where you believe you need to clear errno before 
> calling the function, you found an ill-designed function. Functions should be 
> designed in a way that allows to recognize whether there was an error or not 
> without looking at errno. Errno is only needed to find out _which_ error 
> happened.
> 
> Unfortunately, there is no documentation for uu_list_find(), so it is hard to 
> judge....
> 
> Jörg
>