On Fri, Sep 20, 2013 at 11:00:18AM +0200, Lukas Slebodnik wrote:
On (20/09/13 10:54), Pavel Březina wrote:
>On 09/20/2013 02:30 AM, Simo Sorce wrote:
>>On Thu, 2013-09-19 at 18:45 +0200, Jakub Hrozek wrote:
>>>On Wed, Sep 18, 2013 at 10:40:13PM +0200, Jakub Hrozek wrote:
>>>>On Wed, Sep 18, 2013 at 01:41:36PM -0400, Simo Sorce wrote:
>>>>>On Wed, 2013-09-18 at 17:58 +0200, Jakub Hrozek wrote:
>>>>>>Hi,
>>>>>>
>>>>>>the first patch fixes the bug that Jean-Baptiste Denis found
earlier
>>>>>>today. In case the lookup was not done using a FQDN, the code
would skip
>>>>>>setting the entries to the ncache.
>>>>>>
>>>>>>The second patch is an incremental improvement. I don't think
we should
>>>>>>abort the whole lookup if setting an entry in negcache would
fail. The
>>>>>>negative cache is a performance optimization after all.
>>>>>
>>>>>It seem to me that with the first patch you are changing behavior as
you
>>>>>leave 'ret' unchanged to whatever error is returned instead
of setting
>>>>>it to ENOENT before going to 'done'.
>>>>>
>>>>>Simo.
>>>>
>>>>Ugh, that is a bug.
>>>>
>>>>I was going back and forth on changing the particular return to goto
>>>>and when I made my mind, I forgot to set the errno.
>>>>
>>>>Thanks Simo for catching it, I'll prepare a new version.
>>>
>>>I think we were both confused by the poor label naming. The label actually,
>>>despite being named "done", made the function return ENOENT so the
code
>>>was correct. But even I was confused couple of hours after sending the
>>>patch so the code had to be improved :)
>>>
>>>So in the attached patch I renamed the label to notfound. I hope that's
>>>OK if we don't use the commonly used "done" in this case. I
think the most
>>>important thing is that there is only a single label we jump to.
>>>
>>>Alternatively, if you prefer strictly one exit point, I could convert
>>>the function to only use goto done and set negcache based on errno value
>>>(if ENOENT->ncache) but I didn't see that as necessary.
>>
>>I prefer the idiom:
>> ret = ENOENT;
>> goto done;
>>
>>to:
>> goto notfound;
>>
>>
>>Simo.
>
>I agree. We should not introduce new labels.
Well, I think this particular usage was OK as it was strictly a
"exception", but I won't argue too hard. I also didn't want to change
too much code in a bugfix patch..
I agree too. Compilers are smart enough to do this kind of
optimization.
LS
Yeah, my point was minimal code change during a bugfix that might make
its way even to a stable branch (as this bug affects even sssd-1-9)
rather than optimization..
Anyway, new patches are attached.