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.