Re: [PATCH v5 04/15] pkt-line: add packet_write_gently()
Jeff King writes: > On Wed, Aug 10, 2016 at 03:36:45PM +0200, Lars Schneider wrote: > >> > So now we have packet_write() and packet_write_gently(), but they differ >> > in more than just whether they are gentle. That seems like a weird >> > interface. >> > >> > Should we either be picking a new name (e.g., packet_write_mem() or >> > something), or migrating packet_write() to packet_write_fmt()? >> >> Done in "[PATCH v5 08/15] pkt-line: rename packet_write() to >> packet_write_fmt()" > > Ah, OK. Generally I'd suggest to reorder things so that each patch looks > like a step forward (and so the early patches become preparatory steps, > and the justification in them is something like "we're going to add more > write functions, so let's give this a more descriptive name"). I am guilty for saying "packet_write() should have been similar to write(2)". We may want to have a time-period during which there is no "packet_write()" in the codebase, before we get to that stage. I.e. rename it to packet_write_fmt() to vacate the name and add packet_write_mem(), and then later rename packet_write_mem() to its final name packet_write(), or something like that. The two-step process would reduce the chance of misconversion. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 04/15] pkt-line: add packet_write_gently()
> On 10 Aug 2016, at 15:28, Jeff King wrote:
>
> On Wed, Aug 10, 2016 at 03:04:00PM +0200, larsxschnei...@gmail.com wrote:
>
>> From: Lars Schneider
>>
>> packet_write() has two shortcomings. First, it uses format_packet() which
>> lets the caller only send string data via "%s". That means it cannot be
>> used for arbitrary data that may contain NULs. Second, it will always
>> die on error.
>>
>> Add packet_write_gently() which writes arbitrary data and returns `0` for
>> success and `-1` for an error.
>
> So now we have packet_write() and packet_write_gently(), but they differ
> in more than just whether they are gentle. That seems like a weird
> interface.
>
> Should we either be picking a new name (e.g., packet_write_mem() or
> something), or migrating packet_write() to packet_write_fmt()?
Done in "[PATCH v5 08/15] pkt-line: rename packet_write() to packet_write_fmt()"
>> diff --git a/pkt-line.c b/pkt-line.c
>> index e6b8410..4f25748 100644
>> --- a/pkt-line.c
>> +++ b/pkt-line.c
>> @@ -3,6 +3,7 @@
>> #include "run-command.h"
>>
>> char packet_buffer[LARGE_PACKET_MAX];
>> +char packet_write_buffer[LARGE_PACKET_MAX];
>
> Should this be static? I.e., are random other bits of the code allowed
> to write into it (I guess not because it's not declared in pkt-line.h).
static is better!
>> +int packet_write_gently(const int fd_out, const char *buf, size_t size)
>> +{
>> +if (size > PKTLINE_DATA_MAXLEN)
>> +return -1;
>> +packet_trace(buf, size, 1);
>> +memmove(packet_write_buffer + 4, buf, size);
>
> It looks like this iteration drops the idea of callers using a
> LARGE_PACKET_MAX buffer and only filling it at "buf+4" with
> PKTLINE_DATA_MAXLEN bytes (which is fine).
>
> I wonder if we still need PKTLINE_DATA_MAXLEN, or of it is just
> obscuring things. The magic number "4" still appears separately here,
> and it actually makes it harder to see that things are correct. I.e.,
> doing:
>
> if (size > sizeof(packet_write_buffer) - 4)
> return -1;
> memmove(packet_write_buffer + 4, buf, size);
>
> is more obviously correct, because you do not have to wonder about the
> relationship between the size of your buffer and the macro.
>
> It might still be worth having PKTLINE_DATA_MAXLEN public, though, if
> callers use it to size their input to packet_write_gently().
I agree. In a later patch I am using PKTLINE_DATA_MAXLEN inside pkt-line.c,
too. I will change it to your suggestion.
For now I would remove PKTLINE_DATA_MAXLEN because it should be an
implementation
detail of pkt-line.c (plus it is not used by anyone).
Thanks,
Lars--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 04/15] pkt-line: add packet_write_gently()
From: Lars Schneider
packet_write() has two shortcomings. First, it uses format_packet() which
lets the caller only send string data via "%s". That means it cannot be
used for arbitrary data that may contain NULs. Second, it will always
die on error.
Add packet_write_gently() which writes arbitrary data and returns `0` for
success and `-1` for an error.
Signed-off-by: Lars Schneider
---
pkt-line.c | 12
pkt-line.h | 1 +
2 files changed, 13 insertions(+)
diff --git a/pkt-line.c b/pkt-line.c
index e6b8410..4f25748 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -3,6 +3,7 @@
#include "run-command.h"
char packet_buffer[LARGE_PACKET_MAX];
+char packet_write_buffer[LARGE_PACKET_MAX];
static const char *packet_trace_prefix = "git";
static struct trace_key trace_packet = TRACE_KEY_INIT(PACKET);
static struct trace_key trace_pack = TRACE_KEY_INIT(PACKFILE);
@@ -141,6 +142,17 @@ void packet_write(int fd, const char *fmt, ...)
write_or_die(fd, buf.buf, buf.len);
}
+int packet_write_gently(const int fd_out, const char *buf, size_t size)
+{
+ if (size > PKTLINE_DATA_MAXLEN)
+ return -1;
+ packet_trace(buf, size, 1);
+ memmove(packet_write_buffer + 4, buf, size);
+ size += 4;
+ set_packet_header(packet_write_buffer, size);
+ return (write_in_full(fd_out, packet_write_buffer, size) == size ? 0 :
-1);
+}
+
void packet_buf_write(struct strbuf *buf, const char *fmt, ...)
{
va_list args;
diff --git a/pkt-line.h b/pkt-line.h
index 3cb9d91..88584f1 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -77,6 +77,7 @@ char *packet_read_line_buf(char **src_buf, size_t *src_len,
int *size);
#define DEFAULT_PACKET_MAX 1000
#define LARGE_PACKET_MAX 65520
+#define PKTLINE_DATA_MAXLEN (LARGE_PACKET_MAX - 4)
extern char packet_buffer[LARGE_PACKET_MAX];
#endif
--
2.9.2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 04/15] pkt-line: add packet_write_gently()
On Wed, Aug 10, 2016 at 03:04:00PM +0200, larsxschnei...@gmail.com wrote:
> From: Lars Schneider
>
> packet_write() has two shortcomings. First, it uses format_packet() which
> lets the caller only send string data via "%s". That means it cannot be
> used for arbitrary data that may contain NULs. Second, it will always
> die on error.
>
> Add packet_write_gently() which writes arbitrary data and returns `0` for
> success and `-1` for an error.
So now we have packet_write() and packet_write_gently(), but they differ
in more than just whether they are gentle. That seems like a weird
interface.
Should we either be picking a new name (e.g., packet_write_mem() or
something), or migrating packet_write() to packet_write_fmt()?
> diff --git a/pkt-line.c b/pkt-line.c
> index e6b8410..4f25748 100644
> --- a/pkt-line.c
> +++ b/pkt-line.c
> @@ -3,6 +3,7 @@
> #include "run-command.h"
>
> char packet_buffer[LARGE_PACKET_MAX];
> +char packet_write_buffer[LARGE_PACKET_MAX];
Should this be static? I.e., are random other bits of the code allowed
to write into it (I guess not because it's not declared in pkt-line.h).
> +int packet_write_gently(const int fd_out, const char *buf, size_t size)
> +{
> + if (size > PKTLINE_DATA_MAXLEN)
> + return -1;
> + packet_trace(buf, size, 1);
> + memmove(packet_write_buffer + 4, buf, size);
It looks like this iteration drops the idea of callers using a
LARGE_PACKET_MAX buffer and only filling it at "buf+4" with
PKTLINE_DATA_MAXLEN bytes (which is fine).
I wonder if we still need PKTLINE_DATA_MAXLEN, or of it is just
obscuring things. The magic number "4" still appears separately here,
and it actually makes it harder to see that things are correct. I.e.,
doing:
if (size > sizeof(packet_write_buffer) - 4)
return -1;
memmove(packet_write_buffer + 4, buf, size);
is more obviously correct, because you do not have to wonder about the
relationship between the size of your buffer and the macro.
It might still be worth having PKTLINE_DATA_MAXLEN public, though, if
callers use it to size their input to packet_write_gently().
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 04/15] pkt-line: add packet_write_gently()
> On 10 Aug 2016, at 20:21, Junio C Hamano wrote: > > Lars Schneider writes: > >>> On 10 Aug 2016, at 19:17, Junio C Hamano wrote: >>> >> OK. Does this mean I can leave the "packet_write()" to "packet_write_fmt()" >> rename as is in this series? > > I didn't really check what order you are doing things to answer > that. > > If the function that is introduced in this step is a version of > packet_write_fmt() that does its thing only gently, you would want > to do the rename s/packet_write/packet_write_fmt/ before this step, > and then add the new function as packet_write_fmt_gently(), I would > think. OK - will fix. I did it that way because I thought it would be easier if we decide to drop the big rename patch. Thanks, Lars -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 04/15] pkt-line: add packet_write_gently()
On Wed, Aug 10, 2016 at 03:36:45PM +0200, Lars Schneider wrote: > > So now we have packet_write() and packet_write_gently(), but they differ > > in more than just whether they are gentle. That seems like a weird > > interface. > > > > Should we either be picking a new name (e.g., packet_write_mem() or > > something), or migrating packet_write() to packet_write_fmt()? > > Done in "[PATCH v5 08/15] pkt-line: rename packet_write() to > packet_write_fmt()" Ah, OK. Generally I'd suggest to reorder things so that each patch looks like a step forward (and so the early patches become preparatory steps, and the justification in them is something like "we're going to add more write functions, so let's give this a more descriptive name"). > I agree. In a later patch I am using PKTLINE_DATA_MAXLEN inside pkt-line.c, > too. I will change it to your suggestion. > > For now I would remove PKTLINE_DATA_MAXLEN because it should be an > implementation > detail of pkt-line.c (plus it is not used by anyone). Sounds reasonable. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 04/15] pkt-line: add packet_write_gently()
> On 10 Aug 2016, at 19:17, Junio C Hamano wrote: > > Jeff King writes: > >> On Wed, Aug 10, 2016 at 03:36:45PM +0200, Lars Schneider wrote: >> So now we have packet_write() and packet_write_gently(), but they differ in more than just whether they are gentle. That seems like a weird interface. Should we either be picking a new name (e.g., packet_write_mem() or something), or migrating packet_write() to packet_write_fmt()? >>> >>> Done in "[PATCH v5 08/15] pkt-line: rename packet_write() to >>> packet_write_fmt()" >> >> Ah, OK. Generally I'd suggest to reorder things so that each patch looks >> like a step forward (and so the early patches become preparatory steps, >> and the justification in them is something like "we're going to add more >> write functions, so let's give this a more descriptive name"). > > I am guilty for saying "packet_write() should have been similar to > write(2)". We may want to have a time-period during which there is > no "packet_write()" in the codebase, before we get to that stage. > I.e. rename it to packet_write_fmt() to vacate the name and add > packet_write_mem(), and then later rename packet_write_mem() to its > final name packet_write(), or something like that. The two-step > process would reduce the chance of misconversion. OK. Does this mean I can leave the "packet_write()" to "packet_write_fmt()" rename as is in this series? Thanks, Lars -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 04/15] pkt-line: add packet_write_gently()
Lars Schneider writes: >> On 10 Aug 2016, at 19:17, Junio C Hamano wrote: >> > OK. Does this mean I can leave the "packet_write()" to "packet_write_fmt()" > rename as is in this series? I didn't really check what order you are doing things to answer that. If the function that is introduced in this step is a version of packet_write_fmt() that does its thing only gently, you would want to do the rename s/packet_write/packet_write_fmt/ before this step, and then add the new function as packet_write_fmt_gently(), I would think. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
