Re: [PATCH v7 14/17] run-command: add stdin callback for parallelization
- From
Emily Shaffer <emilyshaffer@google.com>
- Date
- Feb 22, 2021, 23:38 UTC
- Message-ID
- <YDRAbcqtWRpVn703@google.com>
- In-Reply-To
- <20210201065153.1322296-1-jonathantanmy@google.com>
On Sun, Jan 31, 2021 at 10:51:53PM -0800, Jonathan Tan wrote:
Show 42 quoted lines
> > > If a user of the run_processes_parallel() API wants to pipe a large > > amount of information to stdin of each parallel command, that > > information could exceed the buffer of the pipe allocated for that > > process's stdin. Generally this is solved by repeatedly writing to > > child_process.in between calls to start_command() and finish_command(); > > run_processes_parallel() did not provide users an opportunity to access > > child_process at that time. > > [snip] > > > diff --git a/run-command.h b/run-command.h > > index 6472b38bde..e058c0e2c8 100644 > > --- a/run-command.h > > +++ b/run-command.h > > @@ -436,6 +436,20 @@ typedef int (*start_failure_fn)(struct strbuf *out, > > void *pp_cb, > > void *pp_task_cb); > > > > +/** > > + * This callback is called repeatedly on every child process who requests > > + * start_command() to create a pipe by setting child_process.in < 0. > > + * > > + * pp_cb is the callback cookie as passed into run_processes_parallel, and > > + * pp_task_cb is the callback cookie as passed into get_next_task_fn. > > + * The contents of 'send' will be read into the pipe and passed to the pipe. > > + * > > + * Return nonzero to close the pipe. > > + */ > > +typedef int (*feed_pipe_fn)(struct strbuf *pipe, > > + void *pp_cb, > > + void *pp_task_cb); > > + > > As you mention above in the commit message, I think the clearest API to > support what we need is to just have a callback (that has access to > child_process) that is executed between process start and finish. > > As it is, I think this callback is too specific in that it takes a > struct strbuf. I think that this struct strbuf will just end up being > unnecessary copying much of the time, when the user could have just > written to the fd directly.
Since the rest of the run_processes_parallel() API passes strings around with strbufs, I'd prefer to leave it as-is to match the general API expectations and style.
- Emily