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

[Jason King]

On Thu, Jan 20, 2011 at 9:39 AM, Garrett D'Amore <garrett at nexenta.com> wrote:
> 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.

It wouldn't take much to change it.  IIRC, smf is one of the big
consumers of libuutil, so merely booting up a BE with the change would
give it a good workout.

>
>        - 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
>>
>
>
>