> On 07 Apr 2017, at 14:03, Ben Peart <peartben@xxxxxxxxx> wrote: > > Move the sub-proces functions into sub-process.h/c. Add documentation > for the new module in Documentation/technical/api-sub-process.txt > > Signed-off-by: Ben Peart <benpeart@xxxxxxxxxxxxx> > --- > Documentation/technical/api-sub-process.txt | 54 +++++++++++++ > Makefile | 1 + > convert.c | 119 +--------------------------- > sub-process.c | 116 +++++++++++++++++++++++++++ > sub-process.h | 46 +++++++++++ > 5 files changed, 218 insertions(+), 118 deletions(-) > create mode 100644 Documentation/technical/api-sub-process.txt > create mode 100644 sub-process.c > create mode 100644 sub-process.h > > diff --git a/Documentation/technical/api-sub-process.txt b/Documentation/technical/api-sub-process.txt > new file mode 100644 > index 0000000000..eb5005aa72 > --- /dev/null > +++ b/Documentation/technical/api-sub-process.txt > @@ -0,0 +1,54 @@ > +sub-process API > +=============== > + > +The sub-process API makes it possible to run background sub-processes > +that should run until the git command exits and communicate with it > +through stdin and stdout. This reduces the overhead of having to fork > +a new process each time it needs to be communicated with. Minor nit, maybe: The sub-process API makes it possible to run background sub-processes for the entire lifetime of a Git invocation. If Git needs to communicate with an external process multiple times, then this can reduces the process invocation overhead. Git and the sub-process communicate through stdin and stdout. Feel free to ignore as this is bike-shedding. > + > +The sub-processes are kept in a hashmap by command name and looked up > +via the subprocess_find_entry function. If an existing instance can not > +be found then a new process should be created and started. When the > +parent git command terminates, all sub-processes are also terminated. > + > +This API is based on the run-command API. > + > +Data structures > +--------------- > + > +* `struct subprocess_entry` > + > +The sub-process structure. Members should not be accessed directly. > + > +Types > +----- > + > +'int(*subprocess_start_fn)(struct subprocess_entry *entry)':: > + > + User-supplied function to initialize the sub-process. This is > + typically used to negoiate the interface version and capabilities. s/negoiate/negotiate/ > + > + > +Functions > +--------- > + > +`subprocess_start`:: > + > + Start a subprocess and add it to the subprocess hashmap. > + > +`subprocess_stop`:: > + > + Kill a subprocess and remove it from the subprocess hashmap. > + > +`subprocess_find_entry`:: > + > + Find a subprocess in the subprocess hashmap. As mentioned in an earlier review, this function also initializes the hashmap as (maybe to the user unexpected?) side effect. http://public-inbox.org/git/48FA4601-0819-4DE2-943A-7A791BA7C583@xxxxxxxxx/ > + > +`subprocess_get_child_process`:: > + > + Get the underlying `struct child_process` from a subprocess. > + > +`subprocess_read_status`:: > + > + Helper function to read packets looking for the last "status=<foo>" > + key/value pair. > diff --git a/Makefile b/Makefile > index 9f8b35ad41..add945b560 100644 > --- a/Makefile > +++ b/Makefile > @@ -838,6 +838,7 @@ LIB_OBJS += streaming.o > LIB_OBJS += string-list.o > LIB_OBJS += submodule.o > LIB_OBJS += submodule-config.o > +LIB_OBJS += sub-process.o > LIB_OBJS += symlinks.o > LIB_OBJS += tag.o > LIB_OBJS += tempfile.o > diff --git a/convert.c b/convert.c > index 235a6a5279..baa41da760 100644 > --- a/convert.c > +++ b/convert.c > @@ -4,6 +4,7 @@ > #include "quote.h" > #include "sigchain.h" > #include "pkt-line.h" > +#include "sub-process.h" > > /* > * convert.c - convert a file when checking it out and checking it in. > @@ -496,86 +497,11 @@ static int apply_single_file_filter(const char *path, const char *src, size_t le > #define CAP_CLEAN (1u<<0) > #define CAP_SMUDGE (1u<<1) > > -struct subprocess_entry { > - struct hashmap_entry ent; /* must be the first member! */ > - const char *cmd; > - struct child_process process; > -}; > - > struct cmd2process { > struct subprocess_entry subprocess; /* must be the first member! */ > unsigned int supported_capabilities; > }; > > -static int subprocess_map_initialized; > -static struct hashmap subprocess_map; > - > -static int cmd2process_cmp(const struct subprocess_entry *e1, > - const struct subprocess_entry *e2, > - const void *unused) > -{ > - return strcmp(e1->cmd, e2->cmd); > -} > - > -static struct subprocess_entry *subprocess_find_entry(const char *cmd) > -{ > - struct subprocess_entry key; > - > - if (!subprocess_map_initialized) { > - subprocess_map_initialized = 1; > - hashmap_init(&subprocess_map, (hashmap_cmp_fn)cmd2process_cmp, 0); > - return NULL; > - } > - > - hashmap_entry_init(&key, strhash(cmd)); > - key.cmd = cmd; > - return hashmap_get(&subprocess_map, &key, NULL); > -} > - > -static void subprocess_read_status(int fd, struct strbuf *status) > -{ > - struct strbuf **pair; > - char *line; > - for (;;) { > - line = packet_read_line(fd, NULL); > - if (!line) > - break; > - pair = strbuf_split_str(line, '=', 2); > - if (pair[0] && pair[0]->len && pair[1]) { > - /* the last "status=<foo>" line wins */ > - if (!strcmp(pair[0]->buf, "status=")) { > - strbuf_reset(status); > - strbuf_addbuf(status, pair[1]); > - } > - } > - strbuf_list_free(pair); > - } > -} > - > -static void subprocess_stop(struct subprocess_entry *entry) > -{ > - if (!entry) > - return; > - > - entry->process.clean_on_exit = 0; > - kill(entry->process.pid, SIGTERM); > - finish_command(&entry->process); > - > - hashmap_remove(&subprocess_map, entry, NULL); > - free(entry); > -} > - > -static void subprocess_exit_handler(struct child_process *process) > -{ > - sigchain_push(SIGPIPE, SIG_IGN); > - /* Closing the pipe signals the subprocess to initiate a shutdown. */ > - close(process->in); > - close(process->out); > - sigchain_pop(SIGPIPE); > - /* Finish command will wait until the shutdown is complete. */ > - finish_command(process); > -} > - > static int start_multi_file_filter_fn(struct subprocess_entry *subprocess) > { > int err; > @@ -639,49 +565,6 @@ static int start_multi_file_filter_fn(struct subprocess_entry *subprocess) > return err; > } > > -typedef int(*subprocess_start_fn)(struct subprocess_entry *entry); > -int subprocess_start(struct subprocess_entry *entry, const char *cmd, > - subprocess_start_fn startfn) > -{ > - int err; > - struct child_process *process; > - const char *argv[] = { cmd, NULL }; > - > - if (!subprocess_map_initialized) { > - subprocess_map_initialized = 1; > - hashmap_init(&subprocess_map, (hashmap_cmp_fn)cmd2process_cmp, 0); > - } > - > - entry->cmd = cmd; > - process = &entry->process; > - > - child_process_init(process); > - process->argv = argv; > - process->use_shell = 1; > - process->in = -1; > - process->out = -1; > - process->clean_on_exit = 1; > - process->clean_on_exit_handler = subprocess_exit_handler; > - > - err = start_command(process); > - if (err) { > - error("cannot fork to run subprocess '%s'", cmd); > - return err; > - } > - > - hashmap_entry_init(entry, strhash(cmd)); > - > - err = startfn(entry); > - if (err) { > - error("initialization for subprocess '%s' failed", cmd); > - subprocess_stop(entry); > - return err; > - } > - > - hashmap_add(&subprocess_map, entry); > - return 0; > -} > - > static int apply_multi_file_filter(const char *path, const char *src, size_t len, > int fd, struct strbuf *dst, const char *cmd, > const unsigned int wanted_capability) > diff --git a/sub-process.c b/sub-process.c > new file mode 100644 > index 0000000000..60bb650012 > --- /dev/null > +++ b/sub-process.c > @@ -0,0 +1,116 @@ > +/* > + * Generic implementation of background process infrastructure. > + */ > +#include "sub-process.h" > +#include "sigchain.h" > +#include "pkt-line.h" > + > +static int subprocess_map_initialized; > +static struct hashmap subprocess_map; > + > +static int cmd2process_cmp(const struct subprocess_entry *e1, > + const struct subprocess_entry *e2, > + const void *unused) > +{ > + return strcmp(e1->cmd, e2->cmd); > +} > + > +struct subprocess_entry *subprocess_find_entry(const char *cmd) > +{ > + struct subprocess_entry key; > + > + if (!subprocess_map_initialized) { > + subprocess_map_initialized = 1; > + hashmap_init(&subprocess_map, (hashmap_cmp_fn)cmd2process_cmp, 0); > + return NULL; > + } > + > + hashmap_entry_init(&key, strhash(cmd)); > + key.cmd = cmd; > + return hashmap_get(&subprocess_map, &key, NULL); > +} > + > +void subprocess_read_status(int fd, struct strbuf *status) > +{ > + struct strbuf **pair; > + char *line; > + for (;;) { > + line = packet_read_line(fd, NULL); > + if (!line) > + break; > + pair = strbuf_split_str(line, '=', 2); > + if (pair[0] && pair[0]->len && pair[1]) { > + /* the last "status=<foo>" line wins */ > + if (!strcmp(pair[0]->buf, "status=")) { > + strbuf_reset(status); > + strbuf_addbuf(status, pair[1]); > + } > + } > + strbuf_list_free(pair); > + } > +} > + > +void subprocess_stop(struct subprocess_entry *entry) > +{ > + if (!entry) > + return; > + > + entry->process.clean_on_exit = 0; > + kill(entry->process.pid, SIGTERM); > + finish_command(&entry->process); > + > + hashmap_remove(&subprocess_map, entry, NULL); > +} > + > +static void subprocess_exit_handler(struct child_process *process) > +{ > + sigchain_push(SIGPIPE, SIG_IGN); > + /* Closing the pipe signals the subprocess to initiate a shutdown. */ > + close(process->in); > + close(process->out); > + sigchain_pop(SIGPIPE); > + /* Finish command will wait until the shutdown is complete. */ > + finish_command(process); > +} > + > +int subprocess_start(struct subprocess_entry *entry, const char *cmd, > + subprocess_start_fn startfn) > +{ > + int err; > + struct child_process *process; > + const char *argv[] = { cmd, NULL }; > + > + if (!subprocess_map_initialized) { > + subprocess_map_initialized = 1; > + hashmap_init(&subprocess_map, (hashmap_cmp_fn)cmd2process_cmp, 0); > + } > + > + entry->cmd = cmd; > + process = &entry->process; > + > + child_process_init(process); > + process->argv = argv; > + process->use_shell = 1; > + process->in = -1; > + process->out = -1; > + process->clean_on_exit = 1; > + process->clean_on_exit_handler = subprocess_exit_handler; > + > + err = start_command(process); > + if (err) { > + error("cannot fork to run subprocess '%s'", cmd); > + return err; > + } > + > + hashmap_entry_init(entry, strhash(cmd)); > + > + err = startfn(entry); > + if (err) { > + error("initialization for subprocess '%s' failed", cmd); > + subprocess_stop(entry); > + return err; > + } > + > + hashmap_add(&subprocess_map, entry); > + return 0; > +} > diff --git a/sub-process.h b/sub-process.h > new file mode 100644 > index 0000000000..0cf1760a0a > --- /dev/null > +++ b/sub-process.h > @@ -0,0 +1,46 @@ > +#ifndef SUBPROCESS_H > +#define SUBPROCESS_H > + > +#include "git-compat-util.h" > +#include "hashmap.h" > +#include "run-command.h" > + > +/* > + * Generic implementation of background process infrastructure. > + * See Documentation/technical/api-background-process.txt. > + */ > + > + /* data structures */ > + > +struct subprocess_entry { > + struct hashmap_entry ent; /* must be the first member! */ > + const char *cmd; > + struct child_process process; > +}; > + > +/* subprocess functions */ > + > +typedef int(*subprocess_start_fn)(struct subprocess_entry *entry); > +int subprocess_start(struct subprocess_entry *entry, const char *cmd, > + subprocess_start_fn startfn); > + > +void subprocess_stop(struct subprocess_entry *entry); > + > +struct subprocess_entry *subprocess_find_entry(const char *cmd); > + > +/* subprocess helper functions */ > + > +static inline struct child_process *subprocess_get_child_process( > + struct subprocess_entry *entry) > +{ > + return &entry->process; > +} > + > +/* > + * Helper function that will read packets looking for "status=<foo>" > + * key/value pairs and return the value from the last "status" packet > + */ > + > +void subprocess_read_status(int fd, struct strbuf *status); > + > +#endif > -- > 2.12.0.windows.1.31.g1548525701.dirty Looks all good to me. This patch also fixes the failing t0021 "invalid process filter must fail (and not hang!)" introduced here: http://public-inbox.org/git/5900F7F1-89D9-433E-A6C3-0AB27C815BE6@xxxxxxxxx/ TBH, I don't see why this fixes the test, though. Thanks, Lars