Re: [PATCH v6 5/7] pack-bitmap.c: using error() instead of silently returning -1
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Jul 11, 2022, 14:53 UTC
- Message-ID
- <220711.86bktv7l5v.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <52783555e206060465743b5587580a6bd4a1f008.1657540174.git.dyroneteng@gmail.com>
On Mon, Jul 11 2022, Teng Long wrote:
Good to report errno now, however...
Show 15 quoted lines
> Signed-off-by: Teng Long <dyroneteng@gmail.com>
> ---
> pack-bitmap.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/pack-bitmap.c b/pack-bitmap.c
> index 319eb721d8..fbe3f58aff 100644
> --- a/pack-bitmap.c
> +++ b/pack-bitmap.c
> @@ -327,7 +327,7 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
>
> if (fstat(fd, &st)) {
> close(fd);
> - return -1;
> + return error_errno(_("cannot fstat bitmap file"));This is a logic error, as fstat() failed, but you're reproting errno() after our call to close(), at which point it's anyone's guess what "errno" is. It's only meaningful immediately after a system call.
So either:
error_errno(....);
close(fd);
return -1;Or even better:
error_errno(...)
if (close(fd))
warning_errno("cannot close() bitmap file");
return -1;Although that last one may be too pedantic.
Show 21 quoted lines
> }
>
> if (bitmap_git->pack || bitmap_git->midx) {
> @@ -350,8 +350,10 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
> if (load_bitmap_header(bitmap_git) < 0)
> goto cleanup;
>
> - if (!hasheq(get_midx_checksum(bitmap_git->midx), bitmap_git->checksum))
> + if (!hasheq(get_midx_checksum(bitmap_git->midx), bitmap_git->checksum)) {
> + error(_("checksum doesn't match in MIDX and bitmap"));
> goto cleanup;
> + }
>
> if (load_midx_revindex(bitmap_git->midx) < 0) {
> warning(_("multi-pack bitmap is missing required reverse index"));
> @@ -390,7 +392,7 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git
>
> if (fstat(fd, &st)) {
> close(fd);
> - return -1;
> + return error_errno(_("cannot fstat bitmap file"));Same issue here.