diff mbox series

[v4,03/12] pkt-line: (optionally) libify the packet readers

Message ID e05467def4e158a5f1cfa3aafffdb5c77097859a.1613598529.git.gitgitgadget@gmail.com (mailing list archive)
State Accepted
Commit 51b4c9372b14e0cf8f737fe896ad3ec5aa577704
Headers show
Series Simple IPC Mechanism | expand

Commit Message

Johannes Schindelin Feb. 17, 2021, 9:48 p.m. UTC 
From: Johannes Schindelin <johannes.schindelin@gmx.de>

So far, the (possibly indirect) callers of `get_packet_data()` can ask
that function to return an error instead of `die()`ing upon end-of-file.
However, random read errors will still cause the process to die.

So let's introduce an explicit option to tell the packet reader
machinery to please be nice and only return an error.

This change prepares pkt-line for use by long-running daemon processes.
Such processes should be able to serve multiple concurrent clients and
and survive random IO errors.  If there is an error on one connection,
a daemon should be able to drop that connection and continue serving
existing and future connections.

This ability will be used by a Git-aware "Internal FSMonitor" feature
in a later patch series.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 pkt-line.c | 19 +++++++++++++++++--
 pkt-line.h |  4 ++++
 2 files changed, 21 insertions(+), 2 deletions(-)

Comments

Junio C Hamano March 3, 2021, 7:53 p.m. UTC | #1 
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> @@ -313,6 +316,8 @@ static int get_packet_data(int fd, char **src_buf, 
>  		if (options & PACKET_READ_GENTLE_ON_EOF)
>  			return -1;
>  
> +		if (options & PACKET_READ_NEVER_DIE)
> +			return error(_("the remote end hung up unexpectedly"));
>  		die(_("the remote end hung up unexpectedly"));
>  	}

This hunk treats READ_NEVER_DIE as a less quiet version of
GENTRL_ON_EOF, i.e. the new flag allows to continue even after the
"hung up unexpectedly" condition that usually causes the process to
die..

> @@ -355,12 +363,19 @@ enum packet_read_status packet_read_with_status(i
> ...
> -	if ((unsigned)len >= size)
> +	if ((unsigned)len >= size) {
> +		if (options & PACKET_READ_NEVER_DIE)
> +			return error(_("protocol error: bad line length %d"),
> +				     len);
>  		die(_("protocol error: bad line length %d"), len);
> +	}
>  
>  	if (get_packet_data(fd, src_buffer, src_len, buffer, len, options) < 0) {
>  		*pktlen = -1;

In the post-context of this hunk, there is this code:

	if ((options & PACKET_READ_DIE_ON_ERR_PACKET) &&
	    starts_with(buffer, "ERR "))
		die(_("remote error: %s"), buffer + 4);

	*pktlen = len;
	return PACKET_READ_NORMAL;

But here, there is no way to override the DIE_ON_ERR with
READ_NEVER_DIE.

The asymmetry is somewhat annoying (i.e. if "if you do not want to
die upon ERR, don't pass DIE_ON_ERR" could be a valid suggestion to
the callers, then "if you do not want to die upon an unexpected
hung-up, pass GENTLE_ON_EOF" would equally be valid suggestion),
but I'll let it pass.

> diff --git a/pkt-line.h b/pkt-line.h
> index a7149429ac35..2e472efaf2c5 100644
> --- a/pkt-line.h
> +++ b/pkt-line.h
> @@ -75,10 +75,14 @@ int write_packetized_from_buf_no_flush(const char *src_in, size_t len, int fd_ou
>   *
>   * If options contains PACKET_READ_DIE_ON_ERR_PACKET, it dies when it sees an
>   * ERR packet.
> + *
> + * With `PACKET_READ_NEVER_DIE`, no errors are allowed to trigger die() (except
> + * an ERR packet, when `PACKET_READ_DIE_ON_ERR_PACKET` is in effect).
>   */
>  #define PACKET_READ_GENTLE_ON_EOF     (1u<<0)
>  #define PACKET_READ_CHOMP_NEWLINE     (1u<<1)
>  #define PACKET_READ_DIE_ON_ERR_PACKET (1u<<2)
> +#define PACKET_READ_NEVER_DIE         (1u<<3)
>  int packet_read(int fd, char **src_buffer, size_t *src_len, char
>  		*buffer, unsigned size, int options);
Jeff Hostetler March 4, 2021, 2:17 p.m. UTC | #2 
On 3/3/21 2:53 PM, Junio C Hamano wrote:
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
> 
>> @@ -313,6 +316,8 @@ static int get_packet_data(int fd, char **src_buf,
>>   		if (options & PACKET_READ_GENTLE_ON_EOF)
>>   			return -1;
>>   
>> +		if (options & PACKET_READ_NEVER_DIE)
>> +			return error(_("the remote end hung up unexpectedly"));
>>   		die(_("the remote end hung up unexpectedly"));
>>   	}
> 
> This hunk treats READ_NEVER_DIE as a less quiet version of
> GENTRL_ON_EOF, i.e. the new flag allows to continue even after the
> "hung up unexpectedly" condition that usually causes the process to
> die..
> 
>> @@ -355,12 +363,19 @@ enum packet_read_status packet_read_with_status(i
>> ...
>> -	if ((unsigned)len >= size)
>> +	if ((unsigned)len >= size) {
>> +		if (options & PACKET_READ_NEVER_DIE)
>> +			return error(_("protocol error: bad line length %d"),
>> +				     len);
>>   		die(_("protocol error: bad line length %d"), len);
>> +	}
>>   
>>   	if (get_packet_data(fd, src_buffer, src_len, buffer, len, options) < 0) {
>>   		*pktlen = -1;
> 
> In the post-context of this hunk, there is this code:
> 
> 	if ((options & PACKET_READ_DIE_ON_ERR_PACKET) &&
> 	    starts_with(buffer, "ERR "))
> 		die(_("remote error: %s"), buffer + 4);
> 
> 	*pktlen = len;
> 	return PACKET_READ_NORMAL;
> 
> But here, there is no way to override the DIE_ON_ERR with
> READ_NEVER_DIE.
> 
> The asymmetry is somewhat annoying (i.e. if "if you do not want to
> die upon ERR, don't pass DIE_ON_ERR" could be a valid suggestion to
> the callers, then "if you do not want to die upon an unexpected
> hung-up, pass GENTLE_ON_EOF" would equally be valid suggestion),
> but I'll let it pass.

I agree that there is something odd about all of these flags,
but I don't have the context on all the various caller combinations
to make a better suggestion at this time.  And I certainly don't
want to stir up a bigger mess than I already have. :-)

We did document in the .h that READ_NEVER_DIE excludes ERR packets
when READ_DIE_ON_ERR is set, so I think we're safe from unexpected
surprises.

> 
>> diff --git a/pkt-line.h b/pkt-line.h
>> index a7149429ac35..2e472efaf2c5 100644
>> --- a/pkt-line.h
>> +++ b/pkt-line.h
>> @@ -75,10 +75,14 @@ int write_packetized_from_buf_no_flush(const char *src_in, size_t len, int fd_ou
>>    *
>>    * If options contains PACKET_READ_DIE_ON_ERR_PACKET, it dies when it sees an
>>    * ERR packet.
>> + *
>> + * With `PACKET_READ_NEVER_DIE`, no errors are allowed to trigger die() (except
>> + * an ERR packet, when `PACKET_READ_DIE_ON_ERR_PACKET` is in effect).
>>    */
>>   #define PACKET_READ_GENTLE_ON_EOF     (1u<<0)
>>   #define PACKET_READ_CHOMP_NEWLINE     (1u<<1)
>>   #define PACKET_READ_DIE_ON_ERR_PACKET (1u<<2)
>> +#define PACKET_READ_NEVER_DIE         (1u<<3)
>>   int packet_read(int fd, char **src_buffer, size_t *src_len, char
>>   		*buffer, unsigned size, int options);
Jeff King March 4, 2021, 2:40 p.m. UTC | #3 
On Thu, Mar 04, 2021 at 09:17:41AM -0500, Jeff Hostetler wrote:

> > In the post-context of this hunk, there is this code:
> > 
> > 	if ((options & PACKET_READ_DIE_ON_ERR_PACKET) &&
> > 	    starts_with(buffer, "ERR "))
> > 		die(_("remote error: %s"), buffer + 4);
> > 
> > 	*pktlen = len;
> > 	return PACKET_READ_NORMAL;
> > 
> > But here, there is no way to override the DIE_ON_ERR with
> > READ_NEVER_DIE.
> > 
> > The asymmetry is somewhat annoying (i.e. if "if you do not want to
> > die upon ERR, don't pass DIE_ON_ERR" could be a valid suggestion to
> > the callers, then "if you do not want to die upon an unexpected
> > hung-up, pass GENTLE_ON_EOF" would equally be valid suggestion),
> > but I'll let it pass.
> 
> I agree that there is something odd about all of these flags,
> but I don't have the context on all the various caller combinations
> to make a better suggestion at this time.  And I certainly don't
> want to stir up a bigger mess than I already have. :-)
> 
> We did document in the .h that READ_NEVER_DIE excludes ERR packets
> when READ_DIE_ON_ERR is set, so I think we're safe from unexpected
> surprises.

I think the flag is doing sensible things; it's just that the word
"never" in the name is confusing, since it is "never except this one
time".

Would PACKET_READ_GENTLE_ON_READ_ERROR be a better name, to match
GENTLE_ON_EOF? I was tempted to just call it "ON_ERROR", since it also
include parsing errors, but maybe somebody would think that includes ERR
packets (that is more of a stretch, though, I think).

Likewise, I kind of wonder if callers would really prefer suppressing
the error() calls, too. Saying "error: the remote end hung up
unexpectedly" is not that helpful if the "remote end" we are talking
about is fsmonitor, and not the server side of a fetch.

-Peff
Junio C Hamano March 4, 2021, 8:28 p.m. UTC | #4 
Jeff King <peff@peff.net> writes:

> I think the flag is doing sensible things; it's just that the word
> "never" in the name is confusing, since it is "never except this one
> time".
>
> Would PACKET_READ_GENTLE_ON_READ_ERROR be a better name, to match
> GENTLE_ON_EOF? I was tempted to just call it "ON_ERROR", since it also
> include parsing errors, but maybe somebody would think that includes ERR
> packets (that is more of a stretch, though, I think).
>
> Likewise, I kind of wonder if callers would really prefer suppressing
> the error() calls, too. Saying "error: the remote end hung up
> unexpectedly" is not that helpful if the "remote end" we are talking
> about is fsmonitor, and not the server side of a fetch.

Both sounds sensible.
diff mbox series

Patch

diff --git a/pkt-line.c b/pkt-line.c
index 3602b0d37092..83c46e6b46ee 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -304,8 +304,11 @@  static int get_packet_data(int fd, char **src_buf, size_t *src_size,
 		*src_size -= ret;
 	} else {
 		ret = read_in_full(fd, dst, size);
-		if (ret < 0)
+		if (ret < 0) {
+			if (options & PACKET_READ_NEVER_DIE)
+				return error_errno(_("read error"));
 			die_errno(_("read error"));
+		}
 	}
 
 	/* And complain if we didn't get enough bytes to satisfy the read. */
@@ -313,6 +316,8 @@  static int get_packet_data(int fd, char **src_buf, size_t *src_size,
 		if (options & PACKET_READ_GENTLE_ON_EOF)
 			return -1;
 
+		if (options & PACKET_READ_NEVER_DIE)
+			return error(_("the remote end hung up unexpectedly"));
 		die(_("the remote end hung up unexpectedly"));
 	}
 
@@ -341,6 +346,9 @@  enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
 	len = packet_length(linelen);
 
 	if (len < 0) {
+		if (options & PACKET_READ_NEVER_DIE)
+			return error(_("protocol error: bad line length "
+				       "character: %.4s"), linelen);
 		die(_("protocol error: bad line length character: %.4s"), linelen);
 	} else if (!len) {
 		packet_trace("0000", 4, 0);
@@ -355,12 +363,19 @@  enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
 		*pktlen = 0;
 		return PACKET_READ_RESPONSE_END;
 	} else if (len < 4) {
+		if (options & PACKET_READ_NEVER_DIE)
+			return error(_("protocol error: bad line length %d"),
+				     len);
 		die(_("protocol error: bad line length %d"), len);
 	}
 
 	len -= 4;
-	if ((unsigned)len >= size)
+	if ((unsigned)len >= size) {
+		if (options & PACKET_READ_NEVER_DIE)
+			return error(_("protocol error: bad line length %d"),
+				     len);
 		die(_("protocol error: bad line length %d"), len);
+	}
 
 	if (get_packet_data(fd, src_buffer, src_len, buffer, len, options) < 0) {
 		*pktlen = -1;
diff --git a/pkt-line.h b/pkt-line.h
index a7149429ac35..2e472efaf2c5 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -75,10 +75,14 @@  int write_packetized_from_buf_no_flush(const char *src_in, size_t len, int fd_ou
  *
  * If options contains PACKET_READ_DIE_ON_ERR_PACKET, it dies when it sees an
  * ERR packet.
+ *
+ * With `PACKET_READ_NEVER_DIE`, no errors are allowed to trigger die() (except
+ * an ERR packet, when `PACKET_READ_DIE_ON_ERR_PACKET` is in effect).
  */
 #define PACKET_READ_GENTLE_ON_EOF     (1u<<0)
 #define PACKET_READ_CHOMP_NEWLINE     (1u<<1)
 #define PACKET_READ_DIE_ON_ERR_PACKET (1u<<2)
+#define PACKET_READ_NEVER_DIE         (1u<<3)
 int packet_read(int fd, char **src_buffer, size_t *src_len, char
 		*buffer, unsigned size, int options);