Hi Peff,
On Tue, 2 Feb 2021, Jeff King wrote:
> On Mon, Feb 01, 2021 at 07:45:35PM +0000, Jeff Hostetler via GitGitGadget wrote:
>
> > [in packet_write_gently]
> > - set_packet_header(packet_write_buffer, packet_size);
> > - memcpy(packet_write_buffer + 4, buf, size);
> > - if (write_in_full(fd_out, packet_write_buffer, packet_size) < 0)
> > +
> > + set_packet_header(scratch->buffer, packet_size);
> > + memcpy(scratch->buffer + 4, buf, size);
> > +
> > + if (write_in_full(fd_out, scratch->buffer, packet_size) < 0)
> > return error(_("packet write failed"));
>
> Would it really be so bad to do:
>
> char header[4];
> set_packet_header(header, packet_size);
> if (write_in_full(fd_out, header, 4) < 0 ||
> write_in_full(fd_out, buf, size) < 0)
> return error(...);
There must have been a reason why the original code went out of its way to
copy the data. At least that's what I _assume_.
I could see, for example, that these extra round-trips just for the
header, really have a negative impact on network operations.
> I doubt that two syscalls is breaking the bank here, but if people are
> really concerned, using writev() would be a much better solution.
No, because there is no equivalent for that on Windows. And since Windows
is the primary target of our Simple IPC/FSMonitor work, that would break
the bank.
> Obviously we can't rely on it being available everywhere, but it's quite
> easy to emulate with a wrapper (and I'd be happy punt on any writev
> stuff until somebody actually measures a difference).
>
> The other direction is that callers could be using a correctly-sized
> buffer in the first place. I.e., something like:
>
> struct packet_buffer {
> char full_packet[LARGE_PACKET_MAX];
> };
> static inline char *packet_data(struct packet_buffer *pb)
> {
> return pb->full_packet + 4;
> }
Or we change it to
struct packet_buffer {
char count[4];
char payload[LARGE_PACKET_MAX - 4];
};
and then ask the callers to allocate one of those beauties
Not sure how well we can guarantee that the compiler won't pad this,
though.
And then there is `write_packetized_from_buf()` whose `src` parameter can
come from `convert_to_git()` that _definitely_ would not be of the desired
form.
So I guess if we can get away with the 2-syscall version, that's kind of
better than that.
Ciao,
Dscho
>
> That lets people work with the oversized buffer in a natural-ish way
> that would be hard to get wrong, like:
>
> memcpy(packet_data(pb), some_other_buf, len);
>
> (though if we wanted to go even further, we could provide accessors that
> actually do the writing and sanity-check the lengths; the downside is
> that I'm not sure how callers typically get the bytes into these bufs in
> the first place).
>
> That's a much bigger change, of course, and I'd guess you'd much prefer
> to focus on the actual point of your series. ;)
>
> -Peff
>
