| Message ID | 82d4493a6ee7d18063e0feb72d0bc1cc450f2682.1656403084.git.dyroneteng@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tr2: avoid to print "interesting" config repeatedly | expand |
Teng Long <dyroneteng@gmail.com> writes: > if (fstat(fd, &st)) { > close(fd); > - return -1; > + return error_errno(_("cannot stat bitmap file")); "stat" -> "fstat" perhaps? Not a new problem, but before this hunk, we have fd = git_open(...); if (fd < 0) return -1; where it probably is legitimate to ignore ENOENT silently. In practice, it may be OK to treat a file that exists but cannot be opened for whatever reason, but I do not think it would hurt to report such a rare anomaly with a warning, e.g. if (fd < 0) { if (errno != ENOENT) warning("'%s' cannot open '%s'", strerror(errno), bitmap_name); free(bitmap_name); return -1; } or something like that perhaps. > @@ -361,7 +361,7 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git, > bitmap_git->map_pos = 0; > bitmap_git->map = NULL; > bitmap_git->midx = NULL; > - return -1; > + return error(_("cannot open midx bitmap file")); This is not exactly "cannot open". We opened the file, and found there was something we do not like in it. If we failed to find midx lacks revindex chunk, we already would have given a warning, and the error is not just misleading but is redundant. load_bitmap_header() also gives an error() on its own. I think this patch aims for a good end result, but needs more work. At least, checksum mismatch that goes to cleanup also needs to issue its own error() and then we can remove this "cannot open" at the end. > @@ -382,7 +382,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 stat bitmap file")); "stat" -> "fstat" > @@ -394,7 +394,7 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git > > if (!is_pack_valid(packfile)) { > close(fd); > - return -1; > + return error(_("packfile is invalid")); Same "sometimes redundant" comment applies here, but not due to this part of the code but due to the helpers called from is_pack_valid(). Namely, packfile.c::open_packed_git_1() is mostly chatty, but is silent upon "unable to open" and "unable to fstat" (which I think is safe to make chatty as well---we do not want to special case ENOENT in open_packed_git(), so "cannot open because it is not there" is an error). And then, this "invalid" will become redundant and fuzzier message than what is_pack_valid() would have already given, so you can leave it to silently return -1 here. > @@ -409,7 +409,7 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git > bitmap_git->map_size = 0; > bitmap_git->map_pos = 0; > bitmap_git->pack = NULL; > - return -1; > + return error(_("bitmap header is invalid")); Having shown how to analize these kind of things twice in the above, I would not bother checking myself, but you should look at the existing error messages from load_bitmap_header() and see if this extra message should really be here. I suspect not. > } > > return 0;
On 28 Jun 2022 11:04:09 -0700, Junio C Hamano wrote: > "stat" -> "fstat" perhaps? Yes. > Not a new problem, but before this hunk, we have > > fd = git_open(...); > if (fd < 0) > return -1; > > where it probably is legitimate to ignore ENOENT silently. In > practice, it may be OK to treat a file that exists but cannot be > opened for whatever reason, but I do not think it would hurt to > report such a rare anomaly with a warning, e.g. > > if (fd < 0) { > if (errno != ENOENT) > warning("'%s' cannot open '%s'", > strerror(errno), bitmap_name); > free(bitmap_name); > return -1; > } > > or something like that perhaps. Yes. It's more accurate here with your suggestion. At the same time I found there actually exists many similar place like "ignore ENOENT silently" in repo. And I think it's not worth to impove them right now in this patch, if you want to do that it could in another pathset and please tell me. > > @@ -361,7 +361,7 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git, > > bitmap_git->map_pos = 0; > > bitmap_git->map = NULL; > > bitmap_git->midx = NULL; > > - return -1; > > + return error(_("cannot open midx bitmap file")); > > This is not exactly "cannot open". We opened the file, and found > there was something we do not like in it. If we failed to find midx > lacks revindex chunk, we already would have given a warning, and the > error is not just misleading but is redundant. load_bitmap_header() > also gives an error() on its own. > > I think this patch aims for a good end result, but needs more work. > At least, checksum mismatch that goes to cleanup also needs to issue > its own error() and then we can remove this "cannot open" at the end. Yes, It's detailed here and I missed it, I will fix this in next patch: return error() when checksum doesn't match and let "cleanup" return "-1" but not an inaccurate error(). > "stat" -> "fstat" Yes. > > @@ -394,7 +394,7 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git > > > > if (!is_pack_valid(packfile)) { > > close(fd); > > - return -1; > > + return error(_("packfile is invalid")); > > Same "sometimes redundant" comment applies here, but not due to this > part of the code but due to the helpers called from is_pack_valid(). Let me try to know about it, the "helpers" means the place where invoke is_pack_valid() like here. In fact, in is_pack_valid() they already return the errors in various scenarios, so it would be no need to return another error. > Namely, packfile.c::open_packed_git_1() is mostly chatty, but is > silent upon "unable to open" and "unable to fstat" (which I think is > safe to make chatty as well---we do not want to special case ENOENT > in open_packed_git(), so "cannot open because it is not there" is an > error). "chatty" means the code that regularly gives information about its internal operations. Did I understand it right because I'm actually firstly know this description. "cannot open because it is not there" is an error, but I think it will also could be a BUG, actually I'm not very sure for clarify the difference bwtween the use of the two, but I will look into it to dig something out. > And then, this "invalid" will become redundant and fuzzier message > than what is_pack_valid() would have already given, so you can leave > it to silently return -1 here. Clear now. Thanks.
Teng Long <dyroneteng@gmail.com> writes: > It's more accurate here with your suggestion. At the same time I > found there actually exists many similar place like "ignore ENOENT > silently" in repo. And I think it's not worth to impove them right now > in this patch, if you want to do that it could in another pathset and > please tell me. It is sufficien to just mark it as #leftoverbits to find and fix places where the code means to ignore only a missing optional file but ignores all other errors it gets from open/fopen instead. >> > @@ -394,7 +394,7 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git >> > >> > if (!is_pack_valid(packfile)) { >> > close(fd); >> > - return -1; >> > + return error(_("packfile is invalid")); >> >> Same "sometimes redundant" comment applies here, but not due to this >> part of the code but due to the helpers called from is_pack_valid(). > > Let me try to know about it, the "helpers" means the place where invoke > is_pack_valid() like here. In fact, in is_pack_valid() they already > return the errors in various scenarios, so it would be no need to > return another error. Yup. is_pack_valid() calls open_packed_git() and the helper functions that are called from that call chain are full of calls to error() that tell specifically what exactly went wrong. But ... >> Namely, packfile.c::open_packed_git_1() is mostly chatty, but is >> silent upon "unable to open" and "unable to fstat" (which I think is >> safe to make chatty as well---we do not want to special case ENOENT >> in open_packed_git(), so "cannot open because it is not there" is an >> error). ... the error coverage is not complete. There are some (rare) code paths that silently "return -1", not "return error(_("..."))". They should be updated to say something; otherwise we will silently fail in these "rare" codepaths. > "cannot open because it is not there" is an error, but I think it will > also could be a BUG, actually I'm not very sure for clarify the > difference bwtween the use of the two, but I will look into it to dig > something out. The code may have many reasons to believe that a file should exist there and try to open it, but it may find the file missing, but I would suspect that it is never a BUG. You may have run stat() on the path earlier and you know it existed, but by the time you try to open it, some other process may have removed it. You may have found the .idx file and expect that the corresponding .pack file to exist, but the .pack file may be missing. You may have just written a file and you expect to be able to open it, but some other process may have removed it already, or you may have run out of file descriptors and cannot open it. These are all runtime errors that deserve to be reported via error() or die(), but never BUG(). Thanks.
diff --git a/pack-bitmap.c b/pack-bitmap.c index f13a6bfe3a..9f60dbf282 100644 --- a/pack-bitmap.c +++ b/pack-bitmap.c @@ -323,7 +323,7 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git, if (fstat(fd, &st)) { close(fd); - return -1; + return error_errno(_("cannot stat bitmap file")); } if (bitmap_git->pack || bitmap_git->midx) { @@ -361,7 +361,7 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git, bitmap_git->map_pos = 0; bitmap_git->map = NULL; bitmap_git->midx = NULL; - return -1; + return error(_("cannot open midx bitmap file")); } static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git *packfile) @@ -382,7 +382,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 stat bitmap file")); } if (bitmap_git->pack || bitmap_git->midx) { @@ -394,7 +394,7 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git if (!is_pack_valid(packfile)) { close(fd); - return -1; + return error(_("packfile is invalid")); } bitmap_git->pack = packfile; @@ -409,7 +409,7 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git bitmap_git->map_size = 0; bitmap_git->map_pos = 0; bitmap_git->pack = NULL; - return -1; + return error(_("bitmap header is invalid")); } return 0;
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 | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)