| Message ID | 52783555e206060465743b5587580a6bd4a1f008.1657540174.git.dyroneteng@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | trace2: dump scope when print "interesting" config | expand |
On Mon, Jul 11 2022, Teng Long wrote: Good to report errno now, however... > 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. > } > > 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.
Ævar Arnfjörð Bjarmason writes: > 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. > ... > Same issue here. Yes, this will be fixed in next patch. Thanks.
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")); } 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")); } if (bitmap_git->pack || bitmap_git->midx) {
In "open_pack_bitmap_1()" and "open_midx_bitmap_1()", it's better to return error() instead of "-1" when some unexpected error occurs like "stat bitmap file failed", "bitmap header is invalid" or "checksum mismatch", etc. There are places where we do not replace, such as when the bitmap does not exist (no bitmap in repository is allowed) or when another bitmap has already been opened (in which case it should be a warning rather than an error). Signed-off-by: Teng Long <dyroneteng@gmail.com> --- pack-bitmap.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)