Re: [PATCH v7 02/14] csum-file: refactor finalize_hashfile() method
- From
Jakub Narebski <jnareb@gmail.com>
- Date
- Apr 7, 2018, 22:59 UTC
- Message-ID
- <86lgdyei8a.fsf@gmail.com>
- In-Reply-To
- <20180402203427.170177-3-dstolee@microsoft.com>
Derrick Stolee <stolee@gmail.com> writes:
Show 10 quoted lines
> From: Derrick Stolee <dstolee@microsoft.com> > > If we want to use a hashfile on the temporary file for a lockfile, then > we need finalize_hashfile() to fully write the trailing hash but also keep > the file descriptor open. > > Do this by adding a new CSUM_HASH_IN_STREAM flag along with a functional > change that checks this flag before writing the checksum to the stream. > This differs from previous behavior since it would be written if either > CSUM_CLOSE or CSUM_FSYNC is provided.
I'm sorry, but I don't understand from this description what this flag does and what it is meant to do.
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com> > ---
[...]
Show 13 quoted lines
> diff --git a/csum-file.h b/csum-file.h > index 9ba87f0a6c..c5a2e335e7 100644 > --- a/csum-file.h > +++ b/csum-file.h > @@ -27,8 +27,9 @@ extern void hashfile_checkpoint(struct hashfile *, struct hashfile_checkpoint *) > extern int hashfile_truncate(struct hashfile *, struct hashfile_checkpoint *); > > /* finalize_hashfile flags */ > -#define CSUM_CLOSE 1 > -#define CSUM_FSYNC 2 > +#define CSUM_CLOSE 1 > +#define CSUM_FSYNC 2 > +#define CSUM_HASH_IN_STREAM 4
Especially that it is not commented / described here, and the name is unsufficiently descriptive for me.
[...]
Show 12 quoted lines
> diff --git a/csum-file.c b/csum-file.c
> index e6c95a6915..53ce37f7ca 100644
> --- a/csum-file.c
> +++ b/csum-file.c
> @@ -61,11 +61,11 @@ int finalize_hashfile(struct hashfile *f, unsigned char *result, unsigned int fl
> the_hash_algo->final_fn(f->buffer, &f->ctx);
> if (result)
> hashcpy(result, f->buffer);
> - if (flags & (CSUM_CLOSE | CSUM_FSYNC)) {
> - /* write checksum and close fd */
> + if (flags & CSUM_HASH_IN_STREAM)
> flush(f, f->buffer, the_hash_algo->rawsz);Wouldn't CSUM_FLUSH be a better name for this flag?
Show 8 quoted lines
> - if (flags & CSUM_FSYNC)
> - fsync_or_die(f->fd, f->name);
> + if (flags & CSUM_FSYNC)
> + fsync_or_die(f->fd, f->name);
> + if (flags & CSUM_CLOSE) {
> if (close(f->fd))
> die_errno("%s: sha1 file error on close", f->name);
> fd = 0;-- Jakub Narębski