Re: [PATCH] Reopen file descriptor on lookup

[olafBuddenhagen]

Hi,

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.

> +               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...

> +                 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 :-)

-antrik-