Re: [PATCH v8 07/11] pkt-line: add functions to read/write flush terminated packet streams
From: Jakub Narębski <hidden>
Date: 2016-09-25 13:46:56
W dniu 20.09.2016 o 21:02, larsxschneider@gmail.com pisze:
From: Lars Schneider <redacted> write_packetized_from_fd() and write_packetized_from_buf() write a stream of packets. All content packets use the maximal packet size except for the last one. After the last content packet a `flush` control packet is written. read_packetized_to_buf() reads arbitrary sized packets until it detects a `flush` packet.
I guess that read_packetized_to_fd(), for completeness, is not needed for the filter protocol (though it might be useful for the receive side of send-pack / receive-pack). Also, should it be read_packetized_to_strbuf()? I guess using strbuf to read is here because we might need more size to read in full, isn't it.
quoted hunk ↗ jump to hunk
Signed-off-by: Lars Schneider <redacted> --- pkt-line.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ pkt-line.h | 7 +++++++ 2 files changed, 75 insertions(+)diff --git a/pkt-line.c b/pkt-line.c index fc0ac12..a0a8543 100644 --- a/pkt-line.c +++ b/pkt-line.c@@ -196,6 +196,47 @@ void packet_buf_write(struct strbuf *buf, const char *fmt, ...) va_end(args); } +int write_packetized_from_fd(int fd_in, int fd_out)
I wonder if it would be worth it to name parameters in such way that it is known from the name which one is to be packetized, for example fd_out_pkt here... But it might be not worth it; you can get it from the function name.
+{
+ static char buf[PKTLINE_DATA_MAXLEN];Static buffer means not thread-safe and not reentrant. It would be nice to have this information in a comment for this function, but it is not necessary. Also, is using static variable better than using global variable `packet_buffer`? Well, scope for weird interactions is smaller... Sidenote: we have LARGE_PACKET_MAX (used in previous patch), but PKTLINE_DATA_MAXLEN not LARGE_PACKET_DATA_MAX.
+ int err = 0;
+ ssize_t bytes_to_write;
+
+ while (!err) {
+ bytes_to_write = xread(fd_in, buf, sizeof(buf));
+ if (bytes_to_write < 0)
+ return COPY_READ_ERROR;
+ if (bytes_to_write == 0)
+ break;
+ err = packet_write_gently(fd_out, buf, bytes_to_write);
+ }
+ if (!err)
+ err = packet_flush_gently(fd_out);
+ return err;
+}Looks good: clean and readable. Sidenote (probably outside of scope of this patch): what are the errors that we can get from this function, beside COPY_READ_ERROR of course?
+
+int write_packetized_from_buf(const char *src_in, size_t len, int fd_out)
+{
+ static char buf[PKTLINE_DATA_MAXLEN];
+ int err = 0;
+ size_t bytes_written = 0;
+ size_t bytes_to_write;Those two variables, instead of modifying the values of len and/or src_in, make code very easy to read.
+
+ while (!err) {
+ if ((len - bytes_written) > sizeof(buf))
+ bytes_to_write = sizeof(buf);
+ else
+ bytes_to_write = len - bytes_written;
+ if (bytes_to_write == 0)
+ break;
+ err = packet_write_gently(fd_out, src_in + bytes_written, bytes_to_write);
+ bytes_written += bytes_to_write;
+ }
+ if (!err)
+ err = packet_flush_gently(fd_out);
+ return err;
+}Looks good: clean and readable.
quoted hunk ↗ jump to hunk
+ static int get_packet_data(int fd, char **src_buf, size_t *src_size, void *dst, unsigned size, int options) {@@ -305,3 +346,30 @@ char *packet_read_line_buf(char **src, size_t *src_len, int *dst_len) { return packet_read_line_generic(-1, src, src_len, dst_len); } + +ssize_t read_packetized_to_buf(int fd_in, struct strbuf *sb_out)
It's a bit strange that the signature of write_packetized_from_buf() is that different from read_packetized_to_buf(). This includes the return value: int vs ssize_t. As I have checked, write() and read() both use ssize_t, while fread() and fwrite() both use size_t. Perhaps this function should be named read_packetized_to_strbuf() (err, I asked this already)?
+{
+ int paket_len;Possible typo: shouldn't it be called packet_len? Shouldn't it be initialized to 0? + int packet_len = 0;
+ int options = PACKET_READ_GENTLE_ON_EOF;
Why is this even a local variable? It is never changed, and it is used only in one place; we can inline it. If it would be needed in subsequent patches, then such information should be included in the commit message.
+ + size_t oldlen = sb_out->len; + size_t oldalloc = sb_out->alloc;
Just a nitpick (feel free to ignore): doesn't this looks better: + size_t old_len = sb_out->len; + size_t old_alloc = sb_out->alloc; Also perhaps s/old_/orig_/g.
+
+ for (;;) {I see that you used the more popular way of coding forever loop: $ git grep 'for (;;)' -- '*.c' | wc -l 120 $ git grep 'while (1)' -- '*.c' | wc -l 86
+ strbuf_grow(sb_out, PKTLINE_DATA_MAXLEN+1); + paket_len = packet_read(fd_in, NULL, NULL, + sb_out->buf + sb_out->len, PKTLINE_DATA_MAXLEN+1, options);
A question (which perhaps was answered during the development of this patch series): why is this +1 in PKTLINE_DATA_MAXLEN+1 here?
+ if (paket_len <= 0)
+ break;
+ sb_out->len += paket_len;
+ }
+
+ if (paket_len < 0) {
+ if (oldalloc == 0)
+ strbuf_release(sb_out);
+ else
+ strbuf_setlen(sb_out, oldlen);A question (maybe I don't understand strbufs): why there is a special case for oldalloc == 0?
quoted hunk ↗ jump to hunk
+ return paket_len; + } + return sb_out->len - oldlen; +}diff --git a/pkt-line.h b/pkt-line.h index 3fa0899..6df8449 100644 --- a/pkt-line.h +++ b/pkt-line.h@@ -25,6 +25,8 @@ void packet_buf_flush(struct strbuf *buf); void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((format (printf, 2, 3))); int packet_flush_gently(int fd); int packet_write_fmt_gently(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3))); +int write_packetized_from_fd(int fd_in, int fd_out); +int write_packetized_from_buf(const char *src_in, size_t len, int fd_out); /* * Read a packetized line into the buffer, which must be at least size bytes@@ -77,6 +79,11 @@ char *packet_read_line(int fd, int *size); */ char *packet_read_line_buf(char **src_buf, size_t *src_len, int *size); +/* + * Reads a stream of variable sized packets until a flush packet is detected. + */ +ssize_t read_packetized_to_buf(int fd_in, struct strbuf *sb_out); + #define DEFAULT_PACKET_MAX 1000 #define LARGE_PACKET_MAX 65520 extern char packet_buffer[LARGE_PACKET_MAX];