Re: [PATCH] Reopen file descriptor on lookup

[Carl Fredrik Hammar]

Hi,

On Sun, Aug 30, 2009 at 02:50:05PM +0200, olafBuddenhagen@gmx.net wrote:
> On Wed, Aug 26, 2009 at 02:19:28PM +0200, Carl Fredrik Hammar wrote:
> 
> > diff --git a/hurd/lookup-retry.c b/hurd/lookup-retry.c
> > index 96968f8..ce9eaf0 100644
> > --- a/hurd/lookup-retry.c
> > +++ b/hurd/lookup-retry.c
> > @@ -221,15 +221,14 @@ __hurd_file_name_lookup_retry (error_t 
> > (*use_init_port)
> >               errno = save;
> >               if (err)
> >                 return err;
> > -             if (*end == '\0')
> > -               return 0;
> > +             /* Do a normal retry on the remaining components,
> > +                or reopen the descriptor.  */
> 
> As the reopening is part of the normal retry handling AIUI, I don't
> think its helpful to mention it as an explicit "or" clause...
> 
> Perhaps you could mention in the actual if branch that we still need the
> retry when there are no further components, so the descriptor is
> reopened. This would be a more specific statement why we do not return
> like in the original code.

Done.

> > +             if (*end != '\0')
> 
> It's unfortunate IMHO that you reversed the condition: it makes the
> patch much harder to follow. I needed severel minutes to figure out what
> it actually does...

Ok, I undid this.

> > +               file_name = end + 1; /* Skip the slash.  */
> >               else
> > -               {
> > -                 /* Do a normal retry on the remaining components.  */
> > -                 startdir = *result;
> > -                 file_name = end + 1; /* Skip the slash.  */
> > -                 break;
> > -               }
> > +               file_name = end;
> > +             startdir = *result;
> > +             break;
> >             }
> >           else
> >             goto bad_magic;
> 
> Aside from the formal nitpicks above, the patch looks fine, that is, it
> seems to do what it advertises.
> 
> As for the rationale, it looks reasonable to me; but I can not
> ultimately judge it. I guess it's best to submit it to glibc and see
> what happens :-)

Submitted: <http://sourceware.org/ml/libc-alpha/2010-02/msg00042.html>

Regards,
  Fredrik