Re: [PATCH v5 01/35] pkt-line: introduce packet_read_with_status
Brandon Williams writes: >> EOF was -1 and NORMAL was 0 in the previous round; do we need to >> read through all the invocations of functions that return this type >> and make sure there is no "while (such_a_function())" that used to see >> if we read NORMAL that is left un-updated? >> ... >> Will replace. Thanks. > > A reviewer in the previous round found that it was unnecessary to have > EOF start at -1, so per their comments I got rid of that. Yes, I am aware of that exchange, and after vetting the callers I think it is "unnecessary" for EOF to be negative and NORMAL to be 0 with the current code (iow, any value can be used for these enums as long as they are distinct). But that is different matter. If having negative EOF and/or zero NORMAL helps readability of the resulting code, then even if it is not "necessary" for EOF to be negative, it would still be "better".
Re: [PATCH v5 01/35] pkt-line: introduce packet_read_with_status
On 03/14, Junio C Hamano wrote:
> Brandon Williams writes:
>
> > +/*
> > + * 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.
> > + * The number of bytes read will be assigined to *pktlen if the status of
> > the
> > + * read was 'PACKET_READ_NORMAL'.
> > + */
> > +enum packet_read_status {
> > + PACKET_READ_EOF,
> > + PACKET_READ_NORMAL,
> > + PACKET_READ_FLUSH,
> > +};
>
> EOF was -1 and NORMAL was 0 in the previous round; do we need to
> read through all the invocations of functions that return this type
> and make sure there is no "while (such_a_function())" that used to see
> if we read NORMAL that is left un-updated?
>
> I just have gone thru all the hits from
>
> $ git grep -n -e packet_erad_with_status -e packet_reader_read -e
> packet_reader_peek
>
> There are a few
>
> switch (packet_reader_peek())
>
> which by definition we do not have to worry about. Then majority of
> what could be problematic are of the form:
>
> while (packet_reader_read() == PACKET_READ_NORMAL)
>
> and they were this way even in the previous version, so it seems
> quite alright.
>
> Will replace. Thanks.
A reviewer in the previous round found that it was unnecessary to have
EOF start at -1, so per their comments I got rid of that.
--
Brandon Williams
Re: [PATCH v5 01/35] pkt-line: introduce packet_read_with_status
Brandon Williams writes:
> +/*
> + * 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.
> + * The number of bytes read will be assigined to *pktlen if the status of the
> + * read was 'PACKET_READ_NORMAL'.
> + */
> +enum packet_read_status {
> + PACKET_READ_EOF,
> + PACKET_READ_NORMAL,
> + PACKET_READ_FLUSH,
> +};
EOF was -1 and NORMAL was 0 in the previous round; do we need to
read through all the invocations of functions that return this type
and make sure there is no "while (such_a_function())" that used to see
if we read NORMAL that is left un-updated?
I just have gone thru all the hits from
$ git grep -n -e packet_erad_with_status -e packet_reader_read -e
packet_reader_peek
There are a few
switch (packet_reader_peek())
which by definition we do not have to worry about. Then majority of
what could be problematic are of the form:
while (packet_reader_read() == PACKET_READ_NORMAL)
and they were this way even in the previous version, so it seems
quite alright.
Will replace. Thanks.
[PATCH v5 01/35] pkt-line: introduce packet_read_with_status
The current pkt-line API encodes the status of a pkt-line read in the
length of the read content. An error is indicated with '-1', a flush
with '0' (which can be confusing since a return value of '0' can also
indicate an empty pkt-line), and a positive integer for the length of
the read content otherwise. This doesn't leave much room for allowing
the addition of additional special packets in the future.
To solve this introduce 'packet_read_with_status()' which reads a packet
and returns the status of the read encoded as an 'enum packet_status'
type. This allows for easily identifying between special and normal
packets as well as errors. It also enables easily adding a new special
packet in the future.
Signed-off-by: Brandon Williams
---
pkt-line.c | 51 +--
pkt-line.h | 16
2 files changed, 53 insertions(+), 14 deletions(-)
diff --git a/pkt-line.c b/pkt-line.c
index 2827ca772a..db2fb29ac3 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -280,28 +280,39 @@ static int packet_length(const char *linelen)
return (val < 0) ? val : (val << 8) | hex2chr(linelen + 2);
}
-int packet_read(int fd, char **src_buf, size_t *src_len,
- char *buffer, unsigned size, int options)
+enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
+ size_t *src_len, char *buffer,
+ unsigned size, int *pktlen,
+ int options)
{
- int len, ret;
+ int len;
char linelen[4];
- ret = get_packet_data(fd, src_buf, src_len, linelen, 4, options);
- if (ret < 0)
- return ret;
+ if (get_packet_data(fd, src_buffer, src_len, linelen, 4, options) < 0) {
+ *pktlen = -1;
+ return PACKET_READ_EOF;
+ }
+
len = packet_length(linelen);
- if (len < 0)
+
+ if (len < 0) {
die("protocol error: bad line length character: %.4s", linelen);
- if (!len) {
+ } else if (!len) {
packet_trace("", 4, 0);
- return 0;
+ *pktlen = 0;
+ return PACKET_READ_FLUSH;
+ } else if (len < 4) {
+ die("protocol error: bad line length %d", len);
}
+
len -= 4;
- if (len >= size)
+ if ((unsigned)len >= size)
die("protocol error: bad line length %d", len);
- ret = get_packet_data(fd, src_buf, src_len, buffer, len, options);
- if (ret < 0)
- return ret;
+
+ if (get_packet_data(fd, src_buffer, src_len, buffer, len, options) < 0)
{
+ *pktlen = -1;
+ return PACKET_READ_EOF;
+ }
if ((options & PACKET_READ_CHOMP_NEWLINE) &&
len && buffer[len-1] == '\n')
@@ -309,7 +320,19 @@ int packet_read(int fd, char **src_buf, size_t *src_len,
buffer[len] = 0;
packet_trace(buffer, len, 0);
- return len;
+ *pktlen = len;
+ return PACKET_READ_NORMAL;
+}
+
+int packet_read(int fd, char **src_buffer, size_t *src_len,
+ char *buffer, unsigned size, int options)
+{
+ int pktlen = -1;
+
+ packet_read_with_status(fd, src_buffer, src_len, buffer, size,
+ &pktlen, options);
+
+ return pktlen;
}
static char *packet_read_line_generic(int fd,
diff --git a/pkt-line.h b/pkt-line.h
index 3dad583e2d..099b26b95f 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -65,6 +65,22 @@ 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);
+/*
+ * 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.
+ * The number of bytes read will be assigined to *pktlen if the status of the
+ * read was 'PACKET_READ_NORMAL'.
+ */
+enum packet_read_status {
+ PACKET_READ_EOF,
+ PACKET_READ_NORMAL,
+ PACKET_READ_FLUSH,
+};
+enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
+ size_t *src_len, char *buffer,
+ unsigned size, int *pktlen,
+ int options);
+
/*
* Convenience wrapper for packet_read that is not gentle, and sets the
* CHOMP_NEWLINE option. The return value is NULL for a flush packet,
--
2.16.2.804.g6dcf76e118-goog
