Re: [PATCH v7 06/10] pkt-line: add functions to read/write flush terminated packet streams

[Junio C Hamano]

Stefan Beller  writes:

> So as a caller the strbuf is in a different state in case of error
> depending whether
> the strbuf already had some data in it. I think it would be better if
> we only did
> `strbuf_setlen(sb_out, oldlen);` here, such that the caller can
> strbuf_release it
> unconditionally.

If the caller _knows_ that the strbuf is no longer needed, it can
unconditionally call strbuf_release() on it, whether its buffer was
already released or only its length was set to 0, no?

The callee is merely trying to be nice by resetting the strbuf to a
state close to the original in the error return codepath, I would
think.  It may be debatable if such a niceness is needed, but it is
a different matter that does not relate to the burden imposed on the
caller.

Re: [PATCH v7 06/10] pkt-line: add functions to read/write flush terminated packet streams

[Stefan Beller]

On Sun, Sep 11, 2016 at 5:33 AM, Lars Schneider
 wrote:

> Does this convince you to keep the proposed error handling? If yes, then
> I would add a comment to the function to document that behavior explicitly!

oops. I should read the docs more carefully.

Thanks for pointing out.
Then I'd be happy with the patch as is.

Thanks,
Stefan

Re: [PATCH v7 06/10] pkt-line: add functions to read/write flush terminated packet streams

[Lars Schneider]

> On 08 Sep 2016, at 23:49, Stefan Beller  wrote:
> 
> On Thu, Sep 8, 2016 at 11:21 AM,   wrote:
>> From: Lars Schneider 
>> 
>> 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.
> 
> I presume we need both write_* things in a later patch; can you clarify why
> we need both of them?

Since 9035d7 Git streams from fd to required filters and from buf to
non-required filters. The Git filter protocol v2 makes use of all that,
too.

https://github.com/git/git/commit/9035d75a2be9d80d82676504d69553245017f6d4


>> +   if (paket_len < 0) {
>> +   if (oldalloc == 0)
>> +   strbuf_release(sb_out);
> 
> So if old alloc is 0, we release it, which is documented as
> /**
> * Release a string buffer and the memory it used. You should not use the
> * string buffer after using this function, unless you initialize it again.
> */
> 
>> +   else
>> +   strbuf_setlen(sb_out, oldlen);
> 
> Otherwise we just set the length back, such that it looks like before.
> 
> So as a caller the strbuf is in a different state in case of error
> depending whether
> the strbuf already had some data in it. I think it would be better if
> we only did
> `strbuf_setlen(sb_out, oldlen);` here, such that the caller can
> strbuf_release it
> unconditionally.

I tried to mimic the behavior of strbuf_read() [1]. The error handling
was introduced in 2fc647 [2] to ease error handling:

"This allows for easier error handling, as callers only need to call
strbuf_release() if A) the command succeeded or B) if they would have had
to do so anyway because they added something to the strbuf themselves."

Does this convince you to keep the proposed error handling? If yes, then
I would add a comment to the function to document that behavior explicitly!

[1] 
https://github.com/git/git/blob/cda1bbd474805e653dda8a71d4ea3790e2a66cbb/strbuf.c#L377-L383
[2] https://github.com/git/git/commit/2fc647004ac7016128372a85db8245581e493812


> Or to make things more confusing, you could use strbuf_reset in case of 0,
> as that is a strbuf_setlen internally. ;)


Thanks,
Lars

Re: [PATCH v7 06/10] pkt-line: add functions to read/write flush terminated packet streams

[Stefan Beller]

On Thu, Sep 8, 2016 at 11:21 AM,   wrote:
> From: Lars Schneider 
>
> 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.

I presume we need both write_* things in a later patch; can you clarify why
we need both of them?


> +   if (paket_len < 0) {
> +   if (oldalloc == 0)
> +   strbuf_release(sb_out);

So if old alloc is 0, we release it, which is documented as
/**
 * Release a string buffer and the memory it used. You should not use the
 * string buffer after using this function, unless you initialize it again.
 */

> +   else
> +   strbuf_setlen(sb_out, oldlen);

Otherwise we just set the length back, such that it looks like before.

So as a caller the strbuf is in a different state in case of error
depending whether
the strbuf already had some data in it. I think it would be better if
we only did
`strbuf_setlen(sb_out, oldlen);` here, such that the caller can
strbuf_release it
unconditionally.

Or to make things more confusing, you could use strbuf_reset in case of 0,
as that is a strbuf_setlen internally. ;)

> @@ -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.

Strictly speaking we read until a packet of size 0 appears, but as per
the implementation
of packet_read we cannot distinguish between "" and "0004", i.e.
an empty non-flush
packet. So I think we're fine both in the implementation as well as
the documentation here.

[PATCH v7 06/10] pkt-line: add functions to read/write flush terminated packet streams

[larsxschneider]

From: Lars Schneider 

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.

Signed-off-by: Lars Schneider 
---
 pkt-line.c | 68 ++
 pkt-line.h |  7 +++
 2 files changed, 75 insertions(+)

diff --git a/pkt-line.c b/pkt-line.c
index 1d3d725..5001a07 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -209,6 +209,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)
+{
+   static char buf[PKTLINE_DATA_MAXLEN];
+   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;
+}
+
+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;
+
+   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;
+}
+
 static int get_packet_data(int fd, char **src_buf, size_t *src_size,
   void *dst, unsigned size, int options)
 {
@@ -318,3 +359,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)
+{
+   int paket_len;
+   int options = PACKET_READ_GENTLE_ON_EOF;
+
+   size_t oldlen = sb_out->len;
+   size_t oldalloc = sb_out->alloc;
+
+   for (;;) {
+   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);
+   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);
+   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];
-- 
2.10.0