threads / patch / 55500

patchpkt-line: do not report packet write errors twice

Subject: [PATCH] pkt-line: do not report packet write errors twice

## tl;dr

8 messages between Apr 15, 2021 and Apr 15, 2021. Diffs are folded; open one to read it.

replies: 7people: 2as markdown or json

Matheus Tavares· Apr 15, 2021, 19:10 UTC · lore

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(-)
Show changes to pkt-line.c +20 −7
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, ...)
-- 
2.30.1
Junio C Hamano· Apr 15, 2021, 20:06 UTC · re: Matheus Tavares · lore

Re: [PATCH] pkt-line: do not report packet write errors twice

Matheus Tavares <matheus.bernardino@usp.br> writes:
Show 22 quoted lines
> 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(-)
Show changes to diff +8 −4
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;
 }
Matheus Tavares Bernardino· Apr 15, 2021, 20:24 UTC · re: Junio C Hamano · lore

Re: [PATCH] pkt-line: do not report packet write errors twice

On Thu, Apr 15, 2021 at 5:06 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 77 quoted lines
>
> 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.

Junio C Hamano· Apr 15, 2021, 20:32 UTC · re: Matheus Tavares Bernardino · lore

Re: [PATCH] pkt-line: do not report packet write errors twice

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?

Matheus Tavares· Apr 15, 2021, 20:48 UTC · re: Junio C Hamano · lore

Re: [PATCH] pkt-line: do not report packet write errors twice

On Thu, Apr 15, 2021 at 5:32 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 12 quoted lines
>
> 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):

Show changes to pkt-line.c +15 −11
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, ...)
Junio C Hamano· Apr 15, 2021, 20:56 UTC · re: Matheus Tavares · lore

Re: [PATCH] pkt-line: do not report packet write errors twice

Matheus Tavares <matheus.bernardino@usp.br> writes:
Show 17 quoted lines
> 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.
Matheus Tavares· Apr 15, 2021, 21:57 UTC · re: Matheus Tavares · lore

[PATCH v2] pkt-line: do not report packet write errors twice

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 an 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 common lower level function that doesn't print the error message, but instead returns it on a buffer for the caller to die() or error() as appropriate.

Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
---
 pkt-line.c | 31 ++++++++++++++++++++++++-------
 1 file changed, 24 insertions(+), 7 deletions(-)
Show changes to pkt-line.c +24 −7
diff --git a/pkt-line.c b/pkt-line.c
index 0194137528..98304ce374 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -194,13 +194,16 @@ 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,
+			   struct strbuf *err)
 {
 	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) {
+		strbuf_addstr(err, _("packet write failed - data exceeds max packet size"));
+		return -1;
+	}
 
 	packet_trace(buf, size, 1);
 	packet_size = size + 4;
@@ -215,15 +218,29 @@ 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) {
+		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)
+{
+	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)
 {
-	if (packet_write_gently(fd_out, buf, size))
-		die_errno(_("packet write failed"));
+	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, ...)
-- 
2.30.1
Junio C Hamano· Apr 15, 2021, 21:59 UTC · re: Matheus Tavares · lore

Re: [PATCH v2] pkt-line: do not report packet write errors twice

Matheus Tavares <matheus.bernardino@usp.br> writes:
Show 23 quoted lines
> 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 an 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 common lower level function
> that doesn't print the error message, but instead returns it on a buffer
> for the caller to die() or error() as appropriate.
>
> Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
> ---
>  pkt-line.c | 31 ++++++++++++++++++++++++-------
>  1 file changed, 24 insertions(+), 7 deletions(-)
Nicely done.  Thanks, will queue.
Show 59 quoted lines
>
> diff --git a/pkt-line.c b/pkt-line.c
> index 0194137528..98304ce374 100644
> --- a/pkt-line.c
> +++ b/pkt-line.c
> @@ -194,13 +194,16 @@ 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,
> +			   struct strbuf *err)
>  {
>  	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) {
> +		strbuf_addstr(err, _("packet write failed - data exceeds max packet size"));
> +		return -1;
> +	}
>  
>  	packet_trace(buf, size, 1);
>  	packet_size = size + 4;
> @@ -215,15 +218,29 @@ 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) {
> +		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)
> +{
> +	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)
>  {
> -	if (packet_write_gently(fd_out, buf, size))
> -		die_errno(_("packet write failed"));
> +	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, ...)

← back to recent threads