Hi,
On Sun, Aug 30, 2009 at 02:50:05PM +0200, olafbuddenha...@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
