git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: Jeff Hostetler via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Jeff Hostetler" <git@jeffhostetler.com>,
	"Chris Torek" <chris.torek@gmail.com>,
	"Jeff Hostetler" <jeffhost@microsoft.com>
Subject: Re: [PATCH v2 02/14] pkt-line: promote static buffer in packet_write_gently() to callers
Date: Tue, 2 Feb 2021 04:41:53 -0500	[thread overview]
Message-ID: <YBkeYSA5UfQP1m/x@coredump.intra.peff.net> (raw)
In-Reply-To: <3b03a8ff7a72c101f82a685cc6f34a5dd37a9c4b.1612208747.git.gitgitgadget@gmail.com>

On Mon, Feb 01, 2021 at 07:45:35PM +0000, Jeff Hostetler via GitGitGadget wrote:

> -static int packet_write_gently(const int fd_out, const char *buf, size_t size)
> +/*
> + * Use the provided scratch space to build a combined <hdr><buf> buffer
> + * and write it to the file descriptor (in one write if possible).
> + */
> +static int packet_write_gently(const int fd_out, const char *buf, size_t size,
> +			       struct packet_scratch_space *scratch)

Thanks for addressing my stack space concern.

This solution does work (and I like wrapping it in a struct like this),
though I have to wonder if we're not just punting on the thread issues
in an ever-so-slight way with things like this:

>  void packet_write(int fd_out, const char *buf, size_t size)
>  {
> -	if (packet_write_gently(fd_out, buf, size))
> +	static struct packet_scratch_space scratch;
> +
> +	if (packet_write_gently(fd_out, buf, size, &scratch))
>  		die_errno(_("packet write failed"));
>  }

Where we just moved it one step up the call stack.

>  int write_packetized_from_fd(int fd_in, int fd_out)
>  {
> +	/*
> +	 * TODO We could save a memcpy() if we essentially inline
> +	 * TODO packet_write_gently() here and change the xread()
> +	 * TODO to pass &buf[4].
> +	 */

And comments like this make me wonder if the current crop of pktline
functions are just mis-designed in the first place. There are two
obvious directions here.

One, we can observe that the only reason we need the scratch space is to
ship out the whole thing in a single write():

> [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(...);

I doubt that two syscalls is breaking the bank here, but if people are
really concerned, using writev() would be a much better solution.
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;
  }

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

  reply	other threads:[~2021-02-02  9:45 UTC|newest]

Thread overview: 178+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-01-12 15:31 [PATCH 00/10] [RFC] Simple IPC Mechanism Jeff Hostetler via GitGitGadget
2021-01-12 15:31 ` [PATCH 01/10] pkt-line: use stack rather than static buffer in packet_write_gently() Jeff Hostetler via GitGitGadget
2021-01-13 13:29   ` Jeff King
2021-01-25 19:34     ` Jeff Hostetler
2021-01-12 15:31 ` [PATCH 02/10] pkt-line: (optionally) libify the packet readers Johannes Schindelin via GitGitGadget
2021-01-12 15:31 ` [PATCH 03/10] pkt-line: optionally skip the flush packet in write_packetized_from_buf() Johannes Schindelin via GitGitGadget
2021-01-12 15:31 ` [PATCH 04/10] pkt-line: accept additional options in read_packetized_to_strbuf() Johannes Schindelin via GitGitGadget
2021-01-12 15:31 ` [PATCH 05/10] simple-ipc: design documentation for new IPC mechanism Jeff Hostetler via GitGitGadget
2021-01-12 16:40   ` Ævar Arnfjörð Bjarmason
2021-01-12 15:31 ` [PATCH 06/10] simple-ipc: add win32 implementation Jeff Hostetler via GitGitGadget
2021-01-12 15:31 ` [PATCH 07/10] unix-socket: create gentle version of unix_stream_listen() Jeff Hostetler via GitGitGadget
2021-01-13 14:06   ` Jeff King
2021-01-14  1:19     ` Chris Torek
2021-01-12 15:31 ` [PATCH 08/10] unix-socket: add no-chdir option to unix_stream_listen_gently() Jeff Hostetler via GitGitGadget
2021-01-12 15:31 ` [PATCH 09/10] simple-ipc: add t/helper/test-simple-ipc and t0052 Jeff Hostetler via GitGitGadget
2021-01-12 15:31 ` [PATCH 10/10] simple-ipc: add Unix domain socket implementation Jeff Hostetler via GitGitGadget
2021-01-12 16:50 ` [PATCH 00/10] [RFC] Simple IPC Mechanism Ævar Arnfjörð Bjarmason
2021-01-12 18:25   ` Jeff Hostetler
2021-01-12 20:01 ` Junio C Hamano
2021-01-12 23:25   ` Jeff Hostetler
2021-01-13  0:13     ` Junio C Hamano
2021-01-13  0:32       ` Jeff Hostetler
2021-01-13 13:46     ` Jeff King
2021-01-13 15:48       ` Ævar Arnfjörð Bjarmason
2021-02-01 19:45 ` [PATCH v2 00/14] " Jeff Hostetler via GitGitGadget
2021-02-01 19:45   ` [PATCH v2 01/14] ci/install-depends: attempt to fix "brew cask" stuff Junio C Hamano via GitGitGadget
2021-02-01 19:45   ` [PATCH v2 02/14] pkt-line: promote static buffer in packet_write_gently() to callers Jeff Hostetler via GitGitGadget
2021-02-02  9:41     ` Jeff King [this message]
2021-02-02 20:33       ` Jeff Hostetler
2021-02-02 22:54       ` Johannes Schindelin
2021-02-03  4:52         ` Jeff King
2021-02-01 19:45   ` [PATCH v2 03/14] pkt-line: add write_packetized_from_buf2() that takes scratch buffer Jeff Hostetler via GitGitGadget
2021-02-02  9:44     ` Jeff King
2021-02-01 19:45   ` [PATCH v2 04/14] pkt-line: optionally skip the flush packet in write_packetized_from_buf() Johannes Schindelin via GitGitGadget
2021-02-02  9:48     ` Jeff King
2021-02-02 22:56       ` Johannes Schindelin
2021-02-05 18:30       ` Jeff Hostetler
2021-02-01 19:45   ` [PATCH v2 05/14] pkt-line: (optionally) libify the packet readers Johannes Schindelin via GitGitGadget
2021-02-01 19:45   ` [PATCH v2 06/14] pkt-line: accept additional options in read_packetized_to_strbuf() Johannes Schindelin via GitGitGadget
2021-02-11  1:52     ` Taylor Blau
2021-02-01 19:45   ` [PATCH v2 07/14] simple-ipc: design documentation for new IPC mechanism Jeff Hostetler via GitGitGadget
2021-02-01 19:45   ` [PATCH v2 08/14] simple-ipc: add win32 implementation Jeff Hostetler via GitGitGadget
2021-02-01 19:45   ` [PATCH v2 09/14] simple-ipc: add t/helper/test-simple-ipc and t0052 Jeff Hostetler via GitGitGadget
2021-02-02 21:35     ` SZEDER Gábor
2021-02-03  4:36       ` Jeff King
2021-02-09 15:45       ` Jeff Hostetler
2021-02-05 19:38     ` SZEDER Gábor
2021-02-01 19:45   ` [PATCH v2 10/14] unix-socket: elimiate static unix_stream_socket() helper function Jeff Hostetler via GitGitGadget
2021-02-02  9:54     ` Jeff King
2021-02-02  9:58     ` Jeff King
2021-02-01 19:45   ` [PATCH v2 11/14] unix-socket: add options to unix_stream_listen() Jeff Hostetler via GitGitGadget
2021-02-02 10:14     ` Jeff King
2021-02-05 23:28       ` Jeff Hostetler
2021-02-09 16:32         ` Jeff King
2021-02-09 17:39           ` Jeff Hostetler
2021-02-10 15:55             ` Jeff King
2021-02-10 21:31               ` Jeff Hostetler
2021-02-01 19:45   ` [PATCH v2 12/14] unix-socket: add no-chdir option " Jeff Hostetler via GitGitGadget
2021-02-02 10:26     ` Jeff King
2021-02-01 19:45   ` [PATCH v2 13/14] unix-socket: do not call die in unix_stream_connect() Jeff Hostetler via GitGitGadget
2021-02-01 19:45   ` [PATCH v2 14/14] simple-ipc: add Unix domain socket implementation Jeff Hostetler via GitGitGadget
2021-02-01 22:20   ` [PATCH v2 00/14] Simple IPC Mechanism Junio C Hamano
2021-02-01 23:26     ` Jeff Hostetler
2021-02-02 23:07       ` Johannes Schindelin
2021-02-04 19:08         ` Junio C Hamano
2021-02-05 13:19           ` candidate branches for `maint`, was " Johannes Schindelin
2021-02-05 19:55             ` Junio C Hamano
2021-02-13  0:09   ` [PATCH v3 00/12] " Jeff Hostetler via GitGitGadget
2021-02-13  0:09     ` [PATCH v3 01/12] pkt-line: eliminate the need for static buffer in packet_write_gently() Jeff Hostetler via GitGitGadget
2021-02-13  0:09     ` [PATCH v3 02/12] pkt-line: do not issue flush packets in write_packetized_*() Johannes Schindelin via GitGitGadget
2021-02-13  0:09     ` [PATCH v3 03/12] pkt-line: (optionally) libify the packet readers Johannes Schindelin via GitGitGadget
2021-02-13  0:09     ` [PATCH v3 04/12] pkt-line: add options argument to read_packetized_to_strbuf() Johannes Schindelin via GitGitGadget
2021-02-13  0:09     ` [PATCH v3 05/12] simple-ipc: design documentation for new IPC mechanism Jeff Hostetler via GitGitGadget
2021-02-13  0:09     ` [PATCH v3 06/12] simple-ipc: add win32 implementation Jeff Hostetler via GitGitGadget
2021-02-13  0:09     ` [PATCH v3 07/12] unix-socket: elimiate static unix_stream_socket() helper function Jeff Hostetler via GitGitGadget
2021-02-13  0:09     ` [PATCH v3 08/12] unix-socket: add backlog size option to unix_stream_listen() Jeff Hostetler via GitGitGadget
2021-02-13  0:09     ` [PATCH v3 09/12] unix-socket: disallow chdir() when creating unix domain sockets Jeff Hostetler via GitGitGadget
2021-02-13  0:09     ` [PATCH v3 10/12] unix-socket: create `unix_stream_server__listen_with_lock()` Jeff Hostetler via GitGitGadget
2021-02-13  0:09     ` [PATCH v3 11/12] simple-ipc: add Unix domain socket implementation Jeff Hostetler via GitGitGadget
2021-02-13  0:09     ` [PATCH v3 12/12] t0052: add simple-ipc tests and t/helper/test-simple-ipc tool Jeff Hostetler via GitGitGadget
2021-02-13  9:30       ` SZEDER Gábor
2021-02-16 15:53         ` Jeff Hostetler
2021-02-17 21:48     ` [PATCH v4 00/12] Simple IPC Mechanism Jeff Hostetler via GitGitGadget
2021-02-17 21:48       ` [PATCH v4 01/12] pkt-line: eliminate the need for static buffer in packet_write_gently() Jeff Hostetler via GitGitGadget
2021-02-26  7:21         ` Jeff King
2021-02-26 19:52           ` Jeff Hostetler
2021-02-26 20:43             ` Jeff King
2021-03-03 19:38             ` Junio C Hamano
2021-03-04 13:29               ` Jeff Hostetler
2021-03-04 20:26                 ` Junio C Hamano
2021-02-17 21:48       ` [PATCH v4 02/12] pkt-line: do not issue flush packets in write_packetized_*() Johannes Schindelin via GitGitGadget
2021-02-17 21:48       ` [PATCH v4 03/12] pkt-line: (optionally) libify the packet readers Johannes Schindelin via GitGitGadget
2021-03-03 19:53         ` Junio C Hamano
2021-03-04 14:17           ` Jeff Hostetler
2021-03-04 14:40             ` Jeff King
2021-03-04 20:28               ` Junio C Hamano
2021-02-17 21:48       ` [PATCH v4 04/12] pkt-line: add options argument to read_packetized_to_strbuf() Johannes Schindelin via GitGitGadget
2021-02-17 21:48       ` [PATCH v4 05/12] simple-ipc: design documentation for new IPC mechanism Jeff Hostetler via GitGitGadget
2021-03-03 20:19         ` Junio C Hamano
2021-02-17 21:48       ` [PATCH v4 06/12] simple-ipc: add win32 implementation Jeff Hostetler via GitGitGadget
2021-02-17 21:48       ` [PATCH v4 07/12] unix-socket: elimiate static unix_stream_socket() helper function Jeff Hostetler via GitGitGadget
2021-02-26  7:25         ` Jeff King
2021-03-03 20:41         ` Junio C Hamano
2021-02-17 21:48       ` [PATCH v4 08/12] unix-socket: add backlog size option to unix_stream_listen() Jeff Hostetler via GitGitGadget
2021-02-26  7:30         ` Jeff King
2021-03-03 20:54           ` Junio C Hamano
2021-02-17 21:48       ` [PATCH v4 09/12] unix-socket: disallow chdir() when creating unix domain sockets Jeff Hostetler via GitGitGadget
2021-03-03 22:53         ` Junio C Hamano
2021-03-04 14:56           ` Jeff King
2021-03-04 20:34             ` Junio C Hamano
2021-03-04 23:34               ` Junio C Hamano
2021-03-05  9:02                 ` Jeff King
2021-03-05  9:25                   ` Jeff King
2021-03-05 11:59                     ` Chris Torek
2021-03-05 17:33                       ` Jeff Hostetler
2021-03-05 17:53                         ` Junio C Hamano
2021-03-05 21:30               ` Jeff Hostetler
2021-03-05 21:52                 ` Junio C Hamano
2021-02-17 21:48       ` [PATCH v4 10/12] unix-socket: create `unix_stream_server__listen_with_lock()` Jeff Hostetler via GitGitGadget
2021-02-26  7:56         ` Jeff King
2021-03-02 23:50           ` Jeff Hostetler
2021-03-04 15:13             ` Jeff King
2021-02-17 21:48       ` [PATCH v4 11/12] simple-ipc: add Unix domain socket implementation Jeff Hostetler via GitGitGadget
2021-02-17 21:48       ` [PATCH v4 12/12] t0052: add simple-ipc tests and t/helper/test-simple-ipc tool Jeff Hostetler via GitGitGadget
2021-03-02  9:44         ` Jeff King
2021-03-03 15:25           ` Jeff Hostetler
2021-02-25 19:39       ` [PATCH v4 00/12] Simple IPC Mechanism Junio C Hamano
2021-02-26  7:59         ` Jeff King
2021-02-26 20:18           ` Jeff Hostetler
2021-02-26 20:50             ` Jeff King
2021-03-03 19:29               ` Junio C Hamano
2021-03-09 15:02       ` [PATCH v5 " Jeff Hostetler via GitGitGadget
2021-03-09 15:02         ` [PATCH v5 01/12] pkt-line: eliminate the need for static buffer in packet_write_gently() Jeff Hostetler via GitGitGadget
2021-03-09 23:48           ` Junio C Hamano
2021-03-11 19:29             ` Jeff King
2021-03-11 20:32               ` Junio C Hamano
2021-03-11 20:53                 ` Jeff King
2021-03-09 15:02         ` [PATCH v5 02/12] pkt-line: do not issue flush packets in write_packetized_*() Johannes Schindelin via GitGitGadget
2021-03-09 15:02         ` [PATCH v5 03/12] pkt-line: add PACKET_READ_GENTLE_ON_READ_ERROR option Johannes Schindelin via GitGitGadget
2021-03-09 15:02         ` [PATCH v5 04/12] pkt-line: add options argument to read_packetized_to_strbuf() Johannes Schindelin via GitGitGadget
2021-03-09 15:02         ` [PATCH v5 05/12] simple-ipc: design documentation for new IPC mechanism Jeff Hostetler via GitGitGadget
2021-03-09 15:02         ` [PATCH v5 06/12] simple-ipc: add win32 implementation Jeff Hostetler via GitGitGadget
2021-03-09 15:02         ` [PATCH v5 07/12] unix-socket: eliminate static unix_stream_socket() helper function Jeff Hostetler via GitGitGadget
2021-03-09 15:02         ` [PATCH v5 08/12] unix-socket: add backlog size option to unix_stream_listen() Jeff Hostetler via GitGitGadget
2021-03-09 15:02         ` [PATCH v5 09/12] unix-socket: disallow chdir() when creating unix domain sockets Jeff Hostetler via GitGitGadget
2021-03-09 15:02         ` [PATCH v5 10/12] unix-stream-server: create unix domain socket under lock Jeff Hostetler via GitGitGadget
2021-03-10  0:18           ` Junio C Hamano
2021-03-09 15:02         ` [PATCH v5 11/12] simple-ipc: add Unix domain socket implementation Jeff Hostetler via GitGitGadget
2021-03-10  0:08           ` Junio C Hamano
2021-03-15 19:56             ` Jeff Hostetler
2021-03-09 15:02         ` [PATCH v5 12/12] t0052: add simple-ipc tests and t/helper/test-simple-ipc tool Jeff Hostetler via GitGitGadget
2021-03-09 23:28         ` [PATCH v5 00/12] Simple IPC Mechanism Junio C Hamano
2021-03-15 21:08         ` [PATCH v6 " Jeff Hostetler via GitGitGadget
2021-03-15 21:08           ` [PATCH v6 01/12] pkt-line: eliminate the need for static buffer in packet_write_gently() Jeff Hostetler via GitGitGadget
2021-03-15 21:08           ` [PATCH v6 02/12] pkt-line: do not issue flush packets in write_packetized_*() Johannes Schindelin via GitGitGadget
2021-03-15 21:08           ` [PATCH v6 03/12] pkt-line: add PACKET_READ_GENTLE_ON_READ_ERROR option Johannes Schindelin via GitGitGadget
2021-03-15 21:08           ` [PATCH v6 04/12] pkt-line: add options argument to read_packetized_to_strbuf() Johannes Schindelin via GitGitGadget
2021-03-15 21:08           ` [PATCH v6 05/12] simple-ipc: design documentation for new IPC mechanism Jeff Hostetler via GitGitGadget
2021-03-15 21:08           ` [PATCH v6 06/12] simple-ipc: add win32 implementation Jeff Hostetler via GitGitGadget
2021-03-15 21:08           ` [PATCH v6 07/12] unix-socket: eliminate static unix_stream_socket() helper function Jeff Hostetler via GitGitGadget
2021-03-15 21:08           ` [PATCH v6 08/12] unix-socket: add backlog size option to unix_stream_listen() Jeff Hostetler via GitGitGadget
2021-03-15 21:08           ` [PATCH v6 09/12] unix-socket: disallow chdir() when creating unix domain sockets Jeff Hostetler via GitGitGadget
2021-03-15 21:08           ` [PATCH v6 10/12] unix-stream-server: create unix domain socket under lock Jeff Hostetler via GitGitGadget
2021-03-15 21:08           ` [PATCH v6 11/12] simple-ipc: add Unix domain socket implementation Jeff Hostetler via GitGitGadget
2021-03-15 21:08           ` [PATCH v6 12/12] t0052: add simple-ipc tests and t/helper/test-simple-ipc tool Jeff Hostetler via GitGitGadget
2021-03-22 10:29           ` [PATCH v7 00/12] Simple IPC Mechanism Jeff Hostetler via GitGitGadget
2021-03-22 10:29             ` [PATCH v7 01/12] pkt-line: eliminate the need for static buffer in packet_write_gently() Jeff Hostetler via GitGitGadget
2021-03-22 10:29             ` [PATCH v7 02/12] pkt-line: do not issue flush packets in write_packetized_*() Johannes Schindelin via GitGitGadget
2021-03-22 10:29             ` [PATCH v7 03/12] pkt-line: add PACKET_READ_GENTLE_ON_READ_ERROR option Johannes Schindelin via GitGitGadget
2021-03-22 10:29             ` [PATCH v7 04/12] pkt-line: add options argument to read_packetized_to_strbuf() Johannes Schindelin via GitGitGadget
2021-03-22 10:29             ` [PATCH v7 05/12] simple-ipc: design documentation for new IPC mechanism Jeff Hostetler via GitGitGadget
2021-03-22 10:29             ` [PATCH v7 06/12] simple-ipc: add win32 implementation Jeff Hostetler via GitGitGadget
2021-03-22 10:29             ` [PATCH v7 07/12] unix-socket: eliminate static unix_stream_socket() helper function Jeff Hostetler via GitGitGadget
2021-03-22 10:29             ` [PATCH v7 08/12] unix-socket: add backlog size option to unix_stream_listen() Jeff Hostetler via GitGitGadget
2021-03-22 10:29             ` [PATCH v7 09/12] unix-socket: disallow chdir() when creating unix domain sockets Jeff Hostetler via GitGitGadget
2021-03-22 10:29             ` [PATCH v7 10/12] unix-stream-server: create unix domain socket under lock Jeff Hostetler via GitGitGadget
2021-03-22 10:29             ` [PATCH v7 11/12] simple-ipc: add Unix domain socket implementation Jeff Hostetler via GitGitGadget
2021-03-22 10:29             ` [PATCH v7 12/12] t0052: add simple-ipc tests and t/helper/test-simple-ipc tool Jeff Hostetler via GitGitGadget

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YBkeYSA5UfQP1m/x@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=avarab@gmail.com \
    --cc=chris.torek@gmail.com \
    --cc=git@jeffhostetler.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=jeffhost@microsoft.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).