| Message ID | 891a39c853ce3669b6167dc9ad8a2328e4321a9e.1589393036.git.liu.denton@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | remote-curl: partial fix for a deadlock with stateless rpc | expand |
On Wed, May 13, 2020 at 2:07 PM Denton Liu <liu.denton@gmail.com> 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. > > Signed-off-by: Denton Liu <liu.denton@gmail.com> > --- > diff --git 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, > +/* > + * Reads a packetized line and returns the length header of the packet. > + */ > +int packet_length(const char *linelen); The function comment seems rather gobbledy-gooky to me. Perhaps it could be clearer by saying something along the lines of the input being a hexadecimal 2-digit representation of a packet length and that the function converts it to a numeric value (between 0 and 255).
On Wed, May 13, 2020 at 02:04:56PM -0400, Denton Liu wrote: > +/* > + * Reads a packetized line and returns the length header of the packet. > + */ > +int packet_length(const char *linelen); If this is becoming public, I think we need to talk a bit more about: - what is linelen; it's not a NUL-terminated string, which I would have expected from the prototype. It must be at least 4 chars, and doesn't need terminated. - what happens on error; it looks like we return -1 I think the prototype would be more clear to me as: int packet_length(const char *line, size_t len) { if (len < 4) return -1; } which makes it clear that this is a sized buffer. But since nobody should ever be passing anything except "4", it may be overkill. I'd be happy enough with a comment. -Peff
On Fri, May 15, 2020 at 04:56:32PM -0400, Jeff King wrote: > On Wed, May 13, 2020 at 02:04:56PM -0400, Denton Liu wrote: > > > +/* > > + * Reads a packetized line and returns the length header of the packet. > > + */ > > +int packet_length(const char *linelen); > > If this is becoming public, I think we need to talk a bit more about: > > - what is linelen; it's not a NUL-terminated string, which I would > have expected from the prototype. It must be at least 4 chars, and > doesn't need terminated. > > - what happens on error; it looks like we return -1 > > I think the prototype would be more clear to me as: > > int packet_length(const char *line, size_t len) > { > if (len < 4) > return -1; > } > > which makes it clear that this is a sized buffer. But since nobody > should ever be passing anything except "4", it may be overkill. I'd be > happy enough with a comment. Oh, and obviously: int packet_length(const char linelen[4]); would be descriptive, but I think falls afoul of C pointer/array weirdness. -Peff
diff --git a/pkt-line.c b/pkt-line.c index a0e87b1e81..6b60886770 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) { 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..f443185f8f 100644 --- a/pkt-line.h +++ b/pkt-line.h @@ -74,6 +74,11 @@ 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); +/* + * Reads a packetized line and returns the length header of the packet. + */ +int packet_length(const char *linelen); + /* * 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. Signed-off-by: Denton Liu <liu.denton@gmail.com> --- pkt-line.c | 2 +- pkt-line.h | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-)