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. -- Simo Sorce * Red Hat, Inc * New York _______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
