| Message ID | c2b9d033bb774d3bccb572802c3ae114cad8d830.1589816719.git.liu.denton@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | remote-curl: fix deadlocks when remote server disconnects | expand |
On Mon, May 18, 2020 at 11:47:21AM -0400, Denton Liu wrote: > In a future commit, we will be manually processing packets and we will > need to access the length header. In order to simplify this, extern > packet_length() so that the logic can be reused. > > Change the function parameter from a `const char *` to > `const char linelen[4]`. Even though these two types behave identically > as function parameters, use the array notation to semantically indicate > exactly what this function is expecting as an argument. OK. I double-checked that we will not run into any problems with passing pointers or arrays of other sizes (sadly compilers would not tell us if we passed a too-small array, but at least it's documented for humans). > +/* > + * Convert a four hex digit packet line length header into its numeric > + * representation. linelen should not be null-terminated. Minor nit, but it is perfectly fine if there is a NUL. Maybe "linelen does not need to be..."? -Peff
On Mon, May 18, 2020 at 12:04 PM Jeff King <peff@peff.net> wrote: > On Mon, May 18, 2020 at 11:47:21AM -0400, Denton Liu wrote: > > Change the function parameter from a `const char *` to > > `const char linelen[4]`. Even though these two types behave identically > > as function parameters, use the array notation to semantically indicate > > exactly what this function is expecting as an argument. > > > +/* > > + * Convert a four hex digit packet line length header into its numeric > > + * representation. linelen should not be null-terminated. > > Minor nit, but it is perfectly fine if there is a NUL. Maybe "linelen > does not need to be..."? I had the exact same reaction when reading "...should not be null-terminated", however, I'd prefer to drop mention of NUL-termination altogether since talking about it merely confuses the issue since it is quite clear both from the declaration (const char[4]) and the documentation "four hex digits" that 'linelen' is expected to contain exactly four hex digits and no NUL character(s). By the way, I find the argument name "linelen" highly confusing; every time I read it, I think it is an integral type, not a string or character array. I'd very much prefer to see it renamed to "s" (or something) or dropped altogether: int packet_length(const char[4]);
Jeff King <peff@peff.net> writes: >> +/* >> + * Convert a four hex digit packet line length header into its numeric >> + * representation. linelen should not be null-terminated. > > Minor nit, but it is perfectly fine if there is a NUL. Maybe "linelen > does not need to be..."? Yup, I was wondering about the same thing. I actually would go stronger than "does not need to be", as the byte after these four would never be taken as "the terminator for the linelen bytes", even it happens to be '\0'. Just getting rid of the extra sentence would suffice for that, I think. The first sentence makes it clear that it is about interpreting the 4 bytes we are given, and those 4 bytes come from the 'packet line length header' the caller has.
On Mon, May 18, 2020 at 01:50:52PM -0400, Eric Sunshine wrote: > On Mon, May 18, 2020 at 12:04 PM Jeff King <peff@peff.net> wrote: > > On Mon, May 18, 2020 at 11:47:21AM -0400, Denton Liu wrote: > > > Change the function parameter from a `const char *` to > > > `const char linelen[4]`. Even though these two types behave identically > > > as function parameters, use the array notation to semantically indicate > > > exactly what this function is expecting as an argument. > > > > > +/* > > > + * Convert a four hex digit packet line length header into its numeric > > > + * representation. linelen should not be null-terminated. > > > > Minor nit, but it is perfectly fine if there is a NUL. Maybe "linelen > > does not need to be..."? > > I had the exact same reaction when reading "...should not be > null-terminated", however, I'd prefer to drop mention of > NUL-termination altogether since talking about it merely confuses the > issue since it is quite clear both from the declaration (const > char[4]) and the documentation "four hex digits" that 'linelen' is > expected to contain exactly four hex digits and no NUL character(s). Yeah, I think I have a slight preference for that, too. > By the way, I find the argument name "linelen" highly confusing; every > time I read it, I think it is an integral type, not a string or > character array. I'd very much prefer to see it renamed to "s" (or > something) or dropped altogether: > > int packet_length(const char[4]); I think giving it _some_ name is useful. Maybe "const char lenbuf_hex[4]". -Peff
diff --git a/pkt-line.c b/pkt-line.c index a0e87b1e81..5c3b4539b5 100644 --- a/pkt-line.c +++ b/pkt-line.c @@ -306,7 +306,7 @@ static int get_packet_data(int fd, char **src_buf, size_t *src_size, return ret; } -static int packet_length(const char *linelen) +int packet_length(const char linelen[4]) { int val = hex2chr(linelen); return (val < 0) ? val : (val << 8) | hex2chr(linelen + 2); diff --git a/pkt-line.h b/pkt-line.h index fef3a0d792..de81776a7a 100644 --- a/pkt-line.h +++ b/pkt-line.h @@ -74,6 +74,15 @@ int write_packetized_from_buf(const char *src_in, size_t len, int fd_out); int packet_read(int fd, char **src_buffer, size_t *src_len, char *buffer, unsigned size, int options); +/* + * Convert a four hex digit packet line length header into its numeric + * representation. linelen should not be null-terminated. + * + * If linelen contains non-hex characters, return -1. Otherwise, return the + * numeric value of the length header. + */ +int packet_length(const char linelen[4]); + /* * Read a packetized line into a buffer like the 'packet_read()' function but * returns an 'enum packet_read_status' which indicates the status of the read.
In a future commit, we will be manually processing packets and we will need to access the length header. In order to simplify this, extern packet_length() so that the logic can be reused. Change the function parameter from a `const char *` to `const char linelen[4]`. Even though these two types behave identically as function parameters, use the array notation to semantically indicate exactly what this function is expecting as an argument. Signed-off-by: Denton Liu <liu.denton@gmail.com> --- pkt-line.c | 2 +- pkt-line.h | 9 +++++++++ 2 files changed, 10 insertions(+), 1 deletion(-)