W dniu 20.09.2016 o 21:02, larsxschneider@xxxxxxxxx pisze:
> From: Lars Schneider <larsxschneider@xxxxxxxxx>
>
> 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.
>
> Signed-off-by: Lars Schneider <larsxschneider@xxxxxxxxx>
> ---
> 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.
> +
> 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?
> + 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];
>