Re: [PATCH 4/4] resolve_ref_unsafe(): close race condition reading loose refs

From: Jeff King <peff@peff.net>
Date: Jun 12, 2013, 08:04 UTC
Message-ID: <20130612080449.GA537@sigill.intra.peff.net>
In-Reply-To: <1370960780-1055-5-git-send-email-mhagger@alum.mit.edu>

On Tue, Jun 11, 2013 at 04:26:20PM +0200, Michael Haggerty wrote:

Show 6 quoted lines

> Please note that if there is some bizarre filesystem somewhere for
> which, for a single, static file
> 
>     lstat() reports S_ISLNK and readlink() fails with ENOENT or EINVAL
> [...]
> then the inner loop would never terminate.

Yeah, I had the exact same thought while reading your description above. I think we can call that "too crazy to worry about", and deal with it if it ever actually comes up.

Overall your series looks correct to me. The use of the extra "for(;;)" and its control flow feels a little tortured to me. Would it be simpler to just do:

  stat_ref:
          if (lstat(...) < 0)
            ...
          if (readlink(...) < 0)
                  if (errno == ENOENT || errno == EINVAL)
                          /* inconsistent with lstat; retry */
                          goto stat_ref;

          if (open(...) < 0)
                  if (errno == ENOENT)
                          /* inconsistent with lstat; retry */
                          goto stat_ref;

It is really the same thing, but somehow the goto makes it more obvious to me that it is the exceptional case to loop at all (and I think your patch 3/4 could just go away entirely). But I don't mind that much if it stays with the for loop.

Thanks for a pleasant read; the split made the refactoring easy to follow.

-Peff