Re: [PATCH] entry: check for fstat() errors after checkout
- From
Derrick Stolee <stolee@gmail.com>
- Date
- Jul 9, 2020, 11:41 UTC
- Message-ID
- <882592a4-ffb7-924c-b52a-3287b11318b8@gmail.com>
- In-Reply-To
- <fbde5e1c9042ea06b448bc759cea9d45e300961f.1594260597.git.matheus.bernardino@usp.br>
On 7/8/2020 10:10 PM, Matheus Tavares wrote:
Show 9 quoted lines
> In 11179eb311 ("entry.c: check if file exists after checkout",
> 2017-10-05) we started checking the result of the lstat() call done
> after writing a file, to avoid writing garbage to the corresponding
> cache entry. However, the code skips calling lstat() if it's possible
> to use fstat() when it still has the file descriptor open. And when
> calling fstat() we don't do the same error checking. To fix that, let
> the callers of fstat_output() know when fstat() fails. In this case,
> write_entry() will try to use lstat() and properly report an error if
> that fails as well.Looking at this for the first time, I was confused because 11179eb311 doesn't touch these lines. But that's the point: it should have.
Thanks for finding this! I wonder if there is a way to expose this behavior in a test... it definitely seems like this is only something that happens if there is a failure in the filesystem, so I'm not sure such a thing is possible.
It would just be nice to know the ramifications of this change in behavior, keeping in mind that this behavior started way back in e4c7292353 (write_entry(): use fstat() instead of lstat() when file is open, 2009-02-09), over 11 years ago!
Thanks, -Stolee
Show 20 quoted lines
> Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
> ---
> entry.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/entry.c b/entry.c
> index 00b4903366..449bd32dee 100644
> --- a/entry.c
> +++ b/entry.c
> @@ -113,8 +113,7 @@ static int fstat_output(int fd, const struct checkout *state, struct stat *st)
> /* use fstat() only when path == ce->name */
> if (fstat_is_reliable() &&
> state->refresh_cache && !state->base_dir_len) {
> - fstat(fd, st);
> - return 1;
> + return !fstat(fd, st);
> }
> return 0;
> }
>