| Message ID | 64b81865fdfcc505b6aa3e6ebaebf3b9ccb36eb1.1618513583.git.matheus.bernardino@usp.br (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | pkt-line: do not report packet write errors twice | expand |
Matheus Tavares <matheus.bernardino@usp.br> writes: > On write() errors, packet_write() dies with the same error message that > is already printed by its callee, packet_write_gently(). This produces > an unnecessarily verbose and repetitive output: > > error: packet write failed > fatal: packet write failed: <strerror() message> > > In addition to that, packet_write_gently() does not always fulfill its > caller expectation that errno will be properly set before a non-zero > return. In particular, that is not the case for a "data exceeds max > packet size" error. So, in this case, packet_write() will call > die_errno() and print a strerror(errno) message that might be totally > unrelated to the actual error. > > Fix both those issues by turning packet_write() and > packet_write_gently() into wrappers to a lower level function which is > now responsible to either error() or die() as requested by its caller. > > Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br> > --- > pkt-line.c | 27 ++++++++++++++++++++------- > 1 file changed, 20 insertions(+), 7 deletions(-) Nicely done. Duplicated error message literals do look, eh, repetitious, though. I wonder if something like this on top would be an improvement. Upon seeing "return error(_(VARIABLE_NAME))", it may be distracting that you now have to move to where the actual message is defined while following the logic of the code, but as long as the variable name captures the essense of what the message says, it may be OK. I dunno. pkt-line.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git c/pkt-line.c w/pkt-line.c index 39c9ca4212..d357c74fcd 100644 --- c/pkt-line.c +++ w/pkt-line.c @@ -199,12 +199,16 @@ static int do_packet_write(const int fd_out, cons { char header[4]; size_t packet_size; + static const char size_error_message[] = + N_("packet write failed - data exceeds max packet size"); + static const char write_error_message[] = + N_("packet write failed"); if (size > LARGE_PACKET_DATA_MAX) { if (gentle) - return error(_("packet write failed - data exceeds max packet size")); + return error(_(size_error_message)); else - die(_("packet write failed - data exceeds max packet size")); + die(_(size_error_message)); } packet_trace(buf, size, 1); @@ -222,9 +226,9 @@ static int do_packet_write(const int fd_out, const if (write_in_full(fd_out, header, 4) < 0 || write_in_full(fd_out, buf, size) < 0) { if (gentle) - return error_errno(_("packet write failed")); + return error_errno(_(write_error_message)); else - die_errno(_("packet write failed")); + die_errno(_(write_error_message)); } return 0; }
On Thu, Apr 15, 2021 at 5:06 PM Junio C Hamano <gitster@pobox.com> wrote: > > Matheus Tavares <matheus.bernardino@usp.br> writes: > > > On write() errors, packet_write() dies with the same error message that > > is already printed by its callee, packet_write_gently(). This produces > > an unnecessarily verbose and repetitive output: > > > > error: packet write failed > > fatal: packet write failed: <strerror() message> > > > > In addition to that, packet_write_gently() does not always fulfill its > > caller expectation that errno will be properly set before a non-zero > > return. In particular, that is not the case for a "data exceeds max > > packet size" error. So, in this case, packet_write() will call > > die_errno() and print a strerror(errno) message that might be totally > > unrelated to the actual error. > > > > Fix both those issues by turning packet_write() and > > packet_write_gently() into wrappers to a lower level function which is > > now responsible to either error() or die() as requested by its caller. > > > > Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br> > > --- > > pkt-line.c | 27 ++++++++++++++++++++------- > > 1 file changed, 20 insertions(+), 7 deletions(-) > > Nicely done. Duplicated error message literals do look, eh, > repetitious, though. > > I wonder if something like this on top would be an improvement. > > Upon seeing "return error(_(VARIABLE_NAME))", it may be distracting > that you now have to move to where the actual message is defined > while following the logic of the code, but as long as the variable > name captures the essense of what the message says, it may be OK. > > I dunno. > > > pkt-line.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git c/pkt-line.c w/pkt-line.c > index 39c9ca4212..d357c74fcd 100644 > --- c/pkt-line.c > +++ w/pkt-line.c > @@ -199,12 +199,16 @@ static int do_packet_write(const int fd_out, cons > { > char header[4]; > size_t packet_size; > + static const char size_error_message[] = > + N_("packet write failed - data exceeds max packet size"); > + static const char write_error_message[] = > + N_("packet write failed"); > > if (size > LARGE_PACKET_DATA_MAX) { > if (gentle) > - return error(_("packet write failed - data exceeds max packet size")); > + return error(_(size_error_message)); > else > - die(_("packet write failed - data exceeds max packet size")); > + die(_(size_error_message)); > } > > packet_trace(buf, size, 1); > @@ -222,9 +226,9 @@ static int do_packet_write(const int fd_out, const > if (write_in_full(fd_out, header, 4) < 0 || > write_in_full(fd_out, buf, size) < 0) { > if (gentle) > - return error_errno(_("packet write failed")); > + return error_errno(_(write_error_message)); > else > - die_errno(_("packet write failed")); > + die_errno(_(write_error_message)); > } > return 0; > } Nice! :) Maybe we could also avoid the static strings without repeating the literals by making `do_packet_write()` receive a `struct strbuf *err` and save the error message in it? Then the two callers can decide whether to pass it to error() or die() accordingly.
Matheus Tavares Bernardino <matheus.bernardino@usp.br> writes: > Nice! :) Maybe we could also avoid the static strings without > repeating the literals by making `do_packet_write()` receive a `struct > strbuf *err` and save the error message in it? Then the two callers > can decide whether to pass it to error() or die() accordingly. Sorry, but I do not understand what benefit we are trying to gain by introducing an extra strbuf (which I assume is used to strbuf_add() the error message into). Wouldn't the caller need two messages and flip between <error,error_errno> vs <die,die_errno> pair?
On Thu, Apr 15, 2021 at 5:32 PM Junio C Hamano <gitster@pobox.com> wrote: > > Matheus Tavares Bernardino <matheus.bernardino@usp.br> writes: > > > Nice! :) Maybe we could also avoid the static strings without > > repeating the literals by making `do_packet_write()` receive a `struct > > strbuf *err` and save the error message in it? Then the two callers > > can decide whether to pass it to error() or die() accordingly. > > Sorry, but I do not understand what benefit we are trying to gain by > introducing an extra strbuf (which I assume is used to strbuf_add() > the error message into). Wouldn't the caller need two messages and > flip between <error,error_errno> vs <die,die_errno> pair? Hmm, I'm not sure I understand what you mean by the two messages, but what I was thinking of is something like this (on top of my original patch): diff --git a/pkt-line.c b/pkt-line.c index 39c9ca4212..98304ce374 100644 --- a/pkt-line.c +++ b/pkt-line.c @@ -195,16 +195,14 @@ int packet_write_fmt_gently(int fd, const char *fmt, ...) } static int do_packet_write(const int fd_out, const char *buf, size_t size, - int gentle) + struct strbuf *err) { char header[4]; size_t packet_size; if (size > LARGE_PACKET_DATA_MAX) { - if (gentle) - return error(_("packet write failed - data exceeds max packet size")); - else - die(_("packet write failed - data exceeds max packet size")); + strbuf_addstr(err, _("packet write failed - data exceeds max packet size")); + return -1; } packet_trace(buf, size, 1); @@ -221,22 +219,28 @@ static int do_packet_write(const int fd_out, const char *buf, size_t size, if (write_in_full(fd_out, header, 4) < 0 || write_in_full(fd_out, buf, size) < 0) { - if (gentle) - return error_errno(_("packet write failed")); - else - die_errno(_("packet write failed")); + strbuf_addf(err, _("packet write failed: %s"), strerror(errno)); + return -1; } return 0; } static int packet_write_gently(const int fd_out, const char *buf, size_t size) { - return do_packet_write(fd_out, buf, size, 1); + struct strbuf err = STRBUF_INIT; + if (do_packet_write(fd_out, buf, size, &err)) { + error("%s", err.buf); + strbuf_release(&err); + return -1; + } + return 0; } void packet_write(int fd_out, const char *buf, size_t size) { - do_packet_write(fd_out, buf, size, 0); + struct strbuf err = STRBUF_INIT; + if (do_packet_write(fd_out, buf, size, &err)) + die("%s", err.buf); } void packet_buf_write(struct strbuf *buf, const char *fmt, ...)
Matheus Tavares <matheus.bernardino@usp.br> writes: > On Thu, Apr 15, 2021 at 5:32 PM Junio C Hamano <gitster@pobox.com> wrote: >> >> Matheus Tavares Bernardino <matheus.bernardino@usp.br> writes: >> >> > Nice! :) Maybe we could also avoid the static strings without >> > repeating the literals by making `do_packet_write()` receive a `struct >> > strbuf *err` and save the error message in it? Then the two callers >> > can decide whether to pass it to error() or die() accordingly. >> >> Sorry, but I do not understand what benefit we are trying to gain by >> introducing an extra strbuf (which I assume is used to strbuf_add() >> the error message into). Wouldn't the caller need two messages and >> flip between <error,error_errno> vs <die,die_errno> pair? > > Hmm, I'm not sure I understand what you mean by the two messages, but > what I was thinking of is something like this (on top of my original > patch): Ah, OK, you meant that you'd make do_packet_write() handle die/error_errno itself by manually calling strerror(errno). One small downside with the approach is that do_packet_write() needs to hardcode how die/error_errno() mixes the strerror() with the caller supplied error message, but I do not care too strongly either way. My original motivation of suggesting the rewrite was to avoid overlong lines in the main flow of the code by replacing the long messages with variable names that are much shorter, so as long as that is done, I am fine either way. Thanks.
diff --git a/pkt-line.c b/pkt-line.c index 0194137528..39c9ca4212 100644 --- a/pkt-line.c +++ b/pkt-line.c @@ -194,13 +194,18 @@ int packet_write_fmt_gently(int fd, const char *fmt, ...) return status; } -static int packet_write_gently(const int fd_out, const char *buf, size_t size) +static int do_packet_write(const int fd_out, const char *buf, size_t size, + int gentle) { char header[4]; size_t packet_size; - if (size > LARGE_PACKET_DATA_MAX) - return error(_("packet write failed - data exceeds max packet size")); + if (size > LARGE_PACKET_DATA_MAX) { + if (gentle) + return error(_("packet write failed - data exceeds max packet size")); + else + die(_("packet write failed - data exceeds max packet size")); + } packet_trace(buf, size, 1); packet_size = size + 4; @@ -215,15 +220,23 @@ static int packet_write_gently(const int fd_out, const char *buf, size_t size) */ if (write_in_full(fd_out, header, 4) < 0 || - write_in_full(fd_out, buf, size) < 0) - return error(_("packet write failed")); + write_in_full(fd_out, buf, size) < 0) { + if (gentle) + return error_errno(_("packet write failed")); + else + die_errno(_("packet write failed")); + } return 0; } +static int packet_write_gently(const int fd_out, const char *buf, size_t size) +{ + return do_packet_write(fd_out, buf, size, 1); +} + void packet_write(int fd_out, const char *buf, size_t size) { - if (packet_write_gently(fd_out, buf, size)) - die_errno(_("packet write failed")); + do_packet_write(fd_out, buf, size, 0); } void packet_buf_write(struct strbuf *buf, const char *fmt, ...)
On write() errors, packet_write() dies with the same error message that is already printed by its callee, packet_write_gently(). This produces an unnecessarily verbose and repetitive output: error: packet write failed fatal: packet write failed: <strerror() message> In addition to that, packet_write_gently() does not always fulfill its caller expectation that errno will be properly set before a non-zero return. In particular, that is not the case for a "data exceeds max packet size" error. So, in this case, packet_write() will call die_errno() and print a strerror(errno) message that might be totally unrelated to the actual error. Fix both those issues by turning packet_write() and packet_write_gently() into wrappers to a lower level function which is now responsible to either error() or die() as requested by its caller. Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br> --- pkt-line.c | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-)