From 070e5f72d9d6f0056895bd3222e75f8075ed4c0e Mon Sep 17 00:00:00 2001 From: Ben Peart Date: Fri, 5 May 2017 11:27:53 -0400 Subject: [PATCH 01/11] convert: remove erroneous tests for errno == EPIPE start_multi_file_filter() and apply_multi_file_filter() currently test for errno == EPIPE but treating EPIPE as an error is already happening from one of the packet_write() functions. Signed-off-by: Ben Peart Found/Fixed-by: Jeff King Acked-by: Lars Schneider Signed-off-by: Junio C Hamano --- convert.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/convert.c b/convert.c index 8d652bf27c..3908416726 100644 --- a/convert.c +++ b/convert.c @@ -661,7 +661,7 @@ static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, cons done: sigchain_pop(SIGPIPE); - if (err || errno == EPIPE) { + if (err) { error("initialization for external filter '%s' failed", cmd); kill_multi_file_filter(hashmap, entry); return NULL; @@ -752,7 +752,7 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len done: sigchain_pop(SIGPIPE); - if (err || errno == EPIPE) { + if (err) { if (!strcmp(filter_status.buf, "error")) { /* The filter signaled a problem with the file. */ } else if (!strcmp(filter_status.buf, "abort")) { From 974b50c5565107e1e3209176682ee6f6e420bd2b Mon Sep 17 00:00:00 2001 From: Ben Peart Date: Fri, 5 May 2017 11:27:54 -0400 Subject: [PATCH 02/11] pkt-line: fix packet_read_line() to handle len < 0 errors Update packet_read_line() to test for len > 0 to avoid potential bug if read functions return lengths less than zero to indicate errors. Signed-off-by: Ben Peart Found/Fixed-by: Lars Schneider Signed-off-by: Junio C Hamano --- pkt-line.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkt-line.c b/pkt-line.c index d4b6bfe076..6f05b1a4a8 100644 --- a/pkt-line.c +++ b/pkt-line.c @@ -315,7 +315,7 @@ static char *packet_read_line_generic(int fd, PACKET_READ_CHOMP_NEWLINE); if (dst_len) *dst_len = len; - return len ? packet_buffer : NULL; + return (len > 0) ? packet_buffer : NULL; } char *packet_read_line(int fd, int *len_p) From 825b9226bf7a962bc76571319b1444bc02fdcdf8 Mon Sep 17 00:00:00 2001 From: Ben Peart Date: Fri, 5 May 2017 11:27:55 -0400 Subject: [PATCH 03/11] pkt-line: add packet_read_line_gently() Add packet_read_line_gently() to enable reading a line without dying on EOF. Signed-off-by: Ben Peart Signed-off-by: Junio C Hamano --- pkt-line.c | 12 ++++++++++++ pkt-line.h | 11 +++++++++++ 2 files changed, 23 insertions(+) diff --git a/pkt-line.c b/pkt-line.c index 6f05b1a4a8..7db9119573 100644 --- a/pkt-line.c +++ b/pkt-line.c @@ -323,6 +323,18 @@ char *packet_read_line(int fd, int *len_p) return packet_read_line_generic(fd, NULL, NULL, len_p); } +int packet_read_line_gently(int fd, int *dst_len, char **dst_line) +{ + int len = packet_read(fd, NULL, NULL, + packet_buffer, sizeof(packet_buffer), + PACKET_READ_CHOMP_NEWLINE|PACKET_READ_GENTLE_ON_EOF); + if (dst_len) + *dst_len = len; + if (dst_line) + *dst_line = (len > 0) ? packet_buffer : NULL; + return len; +} + char *packet_read_line_buf(char **src, size_t *src_len, int *dst_len) { return packet_read_line_generic(-1, src, src_len, dst_len); diff --git a/pkt-line.h b/pkt-line.h index 18eac64830..66ef610fc4 100644 --- a/pkt-line.h +++ b/pkt-line.h @@ -73,6 +73,17 @@ int packet_read(int fd, char **src_buffer, size_t *src_len, char */ char *packet_read_line(int fd, int *size); +/* + * Convenience wrapper for packet_read that sets the PACKET_READ_GENTLE_ON_EOF + * and CHOMP_NEWLINE options. The return value specifies the number of bytes + * read into the buffer or -1 on truncated input. If the *dst_line parameter + * is not NULL it will return NULL for a flush packet or when the number of + * bytes copied is zero and otherwise points to a static buffer (that may be + * overwritten by subsequent calls). If the size parameter is not NULL, the + * length of the packet is written to it. + */ +int packet_read_line_gently(int fd, int *size, char **dst_line); + /* * Same as packet_read_line, but read from a buf rather than a descriptor; * see packet_read for details on how src_* is used. From c0c70f7ac080c3b4ecc50a3207eb66e97c10271d Mon Sep 17 00:00:00 2001 From: Ben Peart Date: Fri, 5 May 2017 11:27:56 -0400 Subject: [PATCH 04/11] convert: move packet_write_line() into pkt-line as packet_writel() Add packet_writel() which writes multiple lines in a single call and then calls packet_flush_gently(). Update convert.c to use the new packet_writel() function from pkt-line. Signed-off-by: Ben Peart Signed-off-by: Junio C Hamano --- convert.c | 23 ++--------------------- pkt-line.c | 19 +++++++++++++++++++ pkt-line.h | 1 + 3 files changed, 22 insertions(+), 21 deletions(-) diff --git a/convert.c b/convert.c index 3908416726..0f1b5a6340 100644 --- a/convert.c +++ b/convert.c @@ -521,25 +521,6 @@ static struct cmd2process *find_multi_file_filter_entry(struct hashmap *hashmap, return hashmap_get(hashmap, &key, NULL); } -static int packet_write_list(int fd, const char *line, ...) -{ - va_list args; - int err; - va_start(args, line); - for (;;) { - if (!line) - break; - if (strlen(line) > LARGE_PACKET_DATA_MAX) - return -1; - err = packet_write_fmt_gently(fd, "%s\n", line); - if (err) - return err; - line = va_arg(args, const char*); - } - va_end(args); - return packet_flush_gently(fd); -} - static void read_multi_file_filter_status(int fd, struct strbuf *status) { struct strbuf **pair; @@ -616,7 +597,7 @@ static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, cons sigchain_push(SIGPIPE, SIG_IGN); - err = packet_write_list(process->in, "git-filter-client", "version=2", NULL); + err = packet_writel(process->in, "git-filter-client", "version=2", NULL); if (err) goto done; @@ -632,7 +613,7 @@ static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, cons if (err) goto done; - err = packet_write_list(process->in, "capability=clean", "capability=smudge", NULL); + err = packet_writel(process->in, "capability=clean", "capability=smudge", NULL); for (;;) { cap_buf = packet_read_line(process->out, NULL); diff --git a/pkt-line.c b/pkt-line.c index 7db9119573..9d845ecc3c 100644 --- a/pkt-line.c +++ b/pkt-line.c @@ -171,6 +171,25 @@ int packet_write_fmt_gently(int fd, const char *fmt, ...) return status; } +int packet_writel(int fd, const char *line, ...) +{ + va_list args; + int err; + va_start(args, line); + for (;;) { + if (!line) + break; + if (strlen(line) > LARGE_PACKET_DATA_MAX) + return -1; + err = packet_write_fmt_gently(fd, "%s\n", line); + if (err) + return err; + line = va_arg(args, const char*); + } + va_end(args); + return packet_flush_gently(fd); +} + static int packet_write_gently(const int fd_out, const char *buf, size_t size) { static char packet_write_buffer[LARGE_PACKET_MAX]; diff --git a/pkt-line.h b/pkt-line.h index 66ef610fc4..b2965869ad 100644 --- a/pkt-line.h +++ b/pkt-line.h @@ -25,6 +25,7 @@ void packet_buf_flush(struct strbuf *buf); void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((format (printf, 2, 3))); int packet_flush_gently(int fd); int packet_write_fmt_gently(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3))); +int packet_writel(int fd, const char *line, ...); int write_packetized_from_fd(int fd_in, int fd_out); int write_packetized_from_buf(const char *src_in, size_t len, int fd_out); From 7e936842f54c98eb41fb46c94da417065fe333f5 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 13 May 2017 05:04:58 -0400 Subject: [PATCH 05/11] pkt-line: annotate packet_writel with LAST_ARG_MUST_BE_NULL packet_writel() takes a variable-sized list and reads to the first NULL. Let's let the compiler know so that it can help us catch mistakes in the callers. This should have been annotated similarly when it was a static function, but it's doubly important now that the function is available to the whole code-base. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- pkt-line.h | 1 + 1 file changed, 1 insertion(+) diff --git a/pkt-line.h b/pkt-line.h index b2965869ad..450183b649 100644 --- a/pkt-line.h +++ b/pkt-line.h @@ -25,6 +25,7 @@ void packet_buf_flush(struct strbuf *buf); void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((format (printf, 2, 3))); int packet_flush_gently(int fd); int packet_write_fmt_gently(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3))); +LAST_ARG_MUST_BE_NULL int packet_writel(int fd, const char *line, ...); int write_packetized_from_fd(int fd_in, int fd_out); int write_packetized_from_buf(const char *src_in, size_t len, int fd_out); From a810ea9945d5959b6cf73d117bc0f20fde58c556 Mon Sep 17 00:00:00 2001 From: Ben Peart Date: Fri, 5 May 2017 11:27:57 -0400 Subject: [PATCH 06/11] convert: split start_multi_file_filter() into two separate functions To enable future reuse of the filter..process infrastructure, split start_multi_file_filter() into two separate parts. start_multi_file_filter() will now only contain the generic logic to manage the creation and tracking of the child process in a hashmap. start_multi_file_filter_fn() is a protocol specific initialization function that will negotiate the multi-file-filter interface version and capabilities. Signed-off-by: Ben Peart Signed-off-by: Junio C Hamano --- convert.c | 58 ++++++++++++++++++++++++++++++++----------------------- 1 file changed, 34 insertions(+), 24 deletions(-) diff --git a/convert.c b/convert.c index 0f1b5a6340..36401fe087 100644 --- a/convert.c +++ b/convert.c @@ -565,35 +565,14 @@ static void stop_multi_file_filter(struct child_process *process) finish_command(process); } -static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, const char *cmd) +static int start_multi_file_filter_fn(struct cmd2process *entry) { int err; - struct cmd2process *entry; - struct child_process *process; - const char *argv[] = { cmd, NULL }; struct string_list cap_list = STRING_LIST_INIT_NODUP; char *cap_buf; const char *cap_name; - - entry = xmalloc(sizeof(*entry)); - entry->cmd = cmd; - entry->supported_capabilities = 0; - 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 = stop_multi_file_filter; - - if (start_command(process)) { - error("cannot fork to run external filter '%s'", cmd); - return NULL; - } - - hashmap_entry_init(entry, strhash(cmd)); + struct child_process *process = &entry->process; + const char *cmd = entry->cmd; sigchain_push(SIGPIPE, SIG_IGN); @@ -642,6 +621,37 @@ static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, cons done: sigchain_pop(SIGPIPE); + return err; +} + +static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, const char *cmd) +{ + int err; + struct cmd2process *entry; + struct child_process *process; + const char *argv[] = { cmd, NULL }; + + entry = xmalloc(sizeof(*entry)); + entry->cmd = cmd; + entry->supported_capabilities = 0; + 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 = stop_multi_file_filter; + + if (start_command(process)) { + error("cannot fork to run external filter '%s'", cmd); + return NULL; + } + + hashmap_entry_init(entry, strhash(cmd)); + + err = start_multi_file_filter_fn(entry); if (err) { error("initialization for external filter '%s' failed", cmd); kill_multi_file_filter(hashmap, entry); From 1b0b46ee3b310b66023423ba32f023d923d7cb0e Mon Sep 17 00:00:00 2001 From: Ben Peart Date: Fri, 5 May 2017 11:27:58 -0400 Subject: [PATCH 07/11] convert: separate generic structures and variables from the filter specific ones To enable future reuse of the filter..process infrastructure, split the cmd2process structure into two separate parts. subprocess_entry will now contain the generic data required to manage the creation and tracking of the child process in a hashmap. cmd2process is a filter protocol specific structure that is used to track the negotiated capabilities of the filter. Signed-off-by: Ben Peart Signed-off-by: Junio C Hamano --- convert.c | 35 ++++++++++++++++++++--------------- 1 file changed, 20 insertions(+), 15 deletions(-) diff --git a/convert.c b/convert.c index 36401fe087..bfb19beed5 100644 --- a/convert.c +++ b/convert.c @@ -496,26 +496,31 @@ 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 cmd2process { +struct subprocess_entry { struct hashmap_entry ent; /* must be the first member! */ - unsigned int supported_capabilities; const char *cmd; struct child_process process; }; +struct cmd2process { + struct subprocess_entry subprocess; /* must be the first member! */ + unsigned int supported_capabilities; +}; + static int cmd_process_map_initialized; static struct hashmap cmd_process_map; -static int cmd2process_cmp(const struct cmd2process *e1, - const struct cmd2process *e2, +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 cmd2process *find_multi_file_filter_entry(struct hashmap *hashmap, const char *cmd) +static struct subprocess_entry *find_multi_file_filter_entry(struct hashmap *hashmap, const char *cmd) { - struct cmd2process key; + struct subprocess_entry key; + hashmap_entry_init(&key, strhash(cmd)); key.cmd = cmd; return hashmap_get(hashmap, &key, NULL); @@ -541,7 +546,7 @@ static void read_multi_file_filter_status(int fd, struct strbuf *status) } } -static void kill_multi_file_filter(struct hashmap *hashmap, struct cmd2process *entry) +static void kill_multi_file_filter(struct hashmap *hashmap, struct subprocess_entry *entry) { if (!entry) return; @@ -571,8 +576,8 @@ static int start_multi_file_filter_fn(struct cmd2process *entry) struct string_list cap_list = STRING_LIST_INIT_NODUP; char *cap_buf; const char *cap_name; - struct child_process *process = &entry->process; - const char *cmd = entry->cmd; + struct child_process *process = &entry->subprocess.process; + const char *cmd = entry->subprocess.cmd; sigchain_push(SIGPIPE, SIG_IGN); @@ -632,9 +637,9 @@ static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, cons const char *argv[] = { cmd, NULL }; entry = xmalloc(sizeof(*entry)); - entry->cmd = cmd; + entry->subprocess.cmd = cmd; entry->supported_capabilities = 0; - process = &entry->process; + process = &entry->subprocess.process; child_process_init(process); process->argv = argv; @@ -654,7 +659,7 @@ static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, cons err = start_multi_file_filter_fn(entry); if (err) { error("initialization for external filter '%s' failed", cmd); - kill_multi_file_filter(hashmap, entry); + kill_multi_file_filter(hashmap, &entry->subprocess); return NULL; } @@ -678,7 +683,7 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len hashmap_init(&cmd_process_map, (hashmap_cmp_fn) cmd2process_cmp, 0); entry = NULL; } else { - entry = find_multi_file_filter_entry(&cmd_process_map, cmd); + entry = (struct cmd2process *)find_multi_file_filter_entry(&cmd_process_map, cmd); } fflush(NULL); @@ -688,7 +693,7 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len if (!entry) return 0; } - process = &entry->process; + process = &entry->subprocess.process; if (!(wanted_capability & entry->supported_capabilities)) return 0; @@ -759,7 +764,7 @@ done: * Force shutdown and restart if another blob requires filtering. */ error("external filter '%s' failed", cmd); - kill_multi_file_filter(&cmd_process_map, entry); + kill_multi_file_filter(&cmd_process_map, &entry->subprocess); } } else { strbuf_swap(dst, &nbuf); From 7ddb9b2ca9f5a1ebb8a13744575ec3e6879c5f7e Mon Sep 17 00:00:00 2001 From: Ben Peart Date: Fri, 5 May 2017 11:27:59 -0400 Subject: [PATCH 08/11] convert: update generic functions to only use generic data structures Update all functions that are going to be moved into a reusable module so that they only work with the reusable data structures. Move code that is specific to the filter out into the filter specific functions. Signed-off-by: Ben Peart Signed-off-by: Junio C Hamano --- convert.c | 41 +++++++++++++++++++++++------------------ 1 file changed, 23 insertions(+), 18 deletions(-) diff --git a/convert.c b/convert.c index bfb19beed5..d15b10a3c5 100644 --- a/convert.c +++ b/convert.c @@ -556,7 +556,6 @@ static void kill_multi_file_filter(struct hashmap *hashmap, struct subprocess_en finish_command(&entry->process); hashmap_remove(hashmap, entry, NULL); - free(entry); } static void stop_multi_file_filter(struct child_process *process) @@ -570,14 +569,15 @@ static void stop_multi_file_filter(struct child_process *process) finish_command(process); } -static int start_multi_file_filter_fn(struct cmd2process *entry) +static int start_multi_file_filter_fn(struct subprocess_entry *subprocess) { int err; + struct cmd2process *entry = (struct cmd2process *)subprocess; struct string_list cap_list = STRING_LIST_INIT_NODUP; char *cap_buf; const char *cap_name; - struct child_process *process = &entry->subprocess.process; - const char *cmd = entry->subprocess.cmd; + struct child_process *process = &subprocess->process; + const char *cmd = subprocess->cmd; sigchain_push(SIGPIPE, SIG_IGN); @@ -629,17 +629,16 @@ done: return err; } -static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, const char *cmd) +typedef int(*subprocess_start_fn)(struct subprocess_entry *entry); +int start_multi_file_filter(struct hashmap *hashmap, struct subprocess_entry *entry, const char *cmd, + subprocess_start_fn startfn) { int err; - struct cmd2process *entry; struct child_process *process; const char *argv[] = { cmd, NULL }; - entry = xmalloc(sizeof(*entry)); - entry->subprocess.cmd = cmd; - entry->supported_capabilities = 0; - process = &entry->subprocess.process; + entry->cmd = cmd; + process = &entry->process; child_process_init(process); process->argv = argv; @@ -649,22 +648,23 @@ static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, cons process->clean_on_exit = 1; process->clean_on_exit_handler = stop_multi_file_filter; - if (start_command(process)) { + err = start_command(process); + if (err) { error("cannot fork to run external filter '%s'", cmd); - return NULL; + return err; } hashmap_entry_init(entry, strhash(cmd)); - err = start_multi_file_filter_fn(entry); + err = startfn(entry); if (err) { error("initialization for external filter '%s' failed", cmd); - kill_multi_file_filter(hashmap, &entry->subprocess); - return NULL; + kill_multi_file_filter(hashmap, entry); + return err; } hashmap_add(hashmap, entry); - return entry; + return 0; } static int apply_multi_file_filter(const char *path, const char *src, size_t len, @@ -689,9 +689,13 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len fflush(NULL); if (!entry) { - entry = start_multi_file_filter(&cmd_process_map, cmd); - if (!entry) + entry = xmalloc(sizeof(*entry)); + entry->supported_capabilities = 0; + + if (start_multi_file_filter(&cmd_process_map, &entry->subprocess, cmd, start_multi_file_filter_fn)) { + free(entry); return 0; + } } process = &entry->subprocess.process; @@ -765,6 +769,7 @@ done: */ error("external filter '%s' failed", cmd); kill_multi_file_filter(&cmd_process_map, &entry->subprocess); + free(entry); } } else { strbuf_swap(dst, &nbuf); From f514d7d177f7cabbacc3f2cda96ca211266ac2ff Mon Sep 17 00:00:00 2001 From: Ben Peart Date: Fri, 5 May 2017 11:28:00 -0400 Subject: [PATCH 09/11] convert: rename reusable sub-process functions Do a mechanical rename of the functions that will become the reusable sub-process module. Signed-off-by: Ben Peart Signed-off-by: Junio C Hamano --- convert.c | 40 ++++++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/convert.c b/convert.c index d15b10a3c5..cfae45aeee 100644 --- a/convert.c +++ b/convert.c @@ -507,8 +507,8 @@ struct cmd2process { unsigned int supported_capabilities; }; -static int cmd_process_map_initialized; -static struct hashmap cmd_process_map; +static int subprocess_map_initialized; +static struct hashmap subprocess_map; static int cmd2process_cmp(const struct subprocess_entry *e1, const struct subprocess_entry *e2, @@ -517,7 +517,7 @@ static int cmd2process_cmp(const struct subprocess_entry *e1, return strcmp(e1->cmd, e2->cmd); } -static struct subprocess_entry *find_multi_file_filter_entry(struct hashmap *hashmap, const char *cmd) +static struct subprocess_entry *subprocess_find_entry(struct hashmap *hashmap, const char *cmd) { struct subprocess_entry key; @@ -526,7 +526,7 @@ static struct subprocess_entry *find_multi_file_filter_entry(struct hashmap *has return hashmap_get(hashmap, &key, NULL); } -static void read_multi_file_filter_status(int fd, struct strbuf *status) +static void subprocess_read_status(int fd, struct strbuf *status) { struct strbuf **pair; char *line; @@ -546,7 +546,7 @@ static void read_multi_file_filter_status(int fd, struct strbuf *status) } } -static void kill_multi_file_filter(struct hashmap *hashmap, struct subprocess_entry *entry) +static void subprocess_stop(struct hashmap *hashmap, struct subprocess_entry *entry) { if (!entry) return; @@ -558,10 +558,10 @@ static void kill_multi_file_filter(struct hashmap *hashmap, struct subprocess_en hashmap_remove(hashmap, entry, NULL); } -static void stop_multi_file_filter(struct child_process *process) +static void subprocess_exit_handler(struct child_process *process) { sigchain_push(SIGPIPE, SIG_IGN); - /* Closing the pipe signals the filter to initiate a shutdown. */ + /* Closing the pipe signals the subprocess to initiate a shutdown. */ close(process->in); close(process->out); sigchain_pop(SIGPIPE); @@ -630,7 +630,7 @@ done: } typedef int(*subprocess_start_fn)(struct subprocess_entry *entry); -int start_multi_file_filter(struct hashmap *hashmap, struct subprocess_entry *entry, const char *cmd, +int subprocess_start(struct hashmap *hashmap, struct subprocess_entry *entry, const char *cmd, subprocess_start_fn startfn) { int err; @@ -646,11 +646,11 @@ int start_multi_file_filter(struct hashmap *hashmap, struct subprocess_entry *en process->in = -1; process->out = -1; process->clean_on_exit = 1; - process->clean_on_exit_handler = stop_multi_file_filter; + process->clean_on_exit_handler = subprocess_exit_handler; err = start_command(process); if (err) { - error("cannot fork to run external filter '%s'", cmd); + error("cannot fork to run subprocess '%s'", cmd); return err; } @@ -658,8 +658,8 @@ int start_multi_file_filter(struct hashmap *hashmap, struct subprocess_entry *en err = startfn(entry); if (err) { - error("initialization for external filter '%s' failed", cmd); - kill_multi_file_filter(hashmap, entry); + error("initialization for subprocess '%s' failed", cmd); + subprocess_stop(hashmap, entry); return err; } @@ -678,12 +678,12 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len struct strbuf filter_status = STRBUF_INIT; const char *filter_type; - if (!cmd_process_map_initialized) { - cmd_process_map_initialized = 1; - hashmap_init(&cmd_process_map, (hashmap_cmp_fn) cmd2process_cmp, 0); + if (!subprocess_map_initialized) { + subprocess_map_initialized = 1; + hashmap_init(&subprocess_map, (hashmap_cmp_fn) cmd2process_cmp, 0); entry = NULL; } else { - entry = (struct cmd2process *)find_multi_file_filter_entry(&cmd_process_map, cmd); + entry = (struct cmd2process *)subprocess_find_entry(&subprocess_map, cmd); } fflush(NULL); @@ -692,7 +692,7 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len entry = xmalloc(sizeof(*entry)); entry->supported_capabilities = 0; - if (start_multi_file_filter(&cmd_process_map, &entry->subprocess, cmd, start_multi_file_filter_fn)) { + if (subprocess_start(&subprocess_map, &entry->subprocess, cmd, start_multi_file_filter_fn)) { free(entry); return 0; } @@ -737,7 +737,7 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len if (err) goto done; - read_multi_file_filter_status(process->out, &filter_status); + subprocess_read_status(process->out, &filter_status); err = strcmp(filter_status.buf, "success"); if (err) goto done; @@ -746,7 +746,7 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len if (err) goto done; - read_multi_file_filter_status(process->out, &filter_status); + subprocess_read_status(process->out, &filter_status); err = strcmp(filter_status.buf, "success"); done: @@ -768,7 +768,7 @@ done: * Force shutdown and restart if another blob requires filtering. */ error("external filter '%s' failed", cmd); - kill_multi_file_filter(&cmd_process_map, &entry->subprocess); + subprocess_stop(&subprocess_map, &entry->subprocess); free(entry); } } else { From 99605d62e8e7e568035dc953b24b79b3d52f0522 Mon Sep 17 00:00:00 2001 From: Ben Peart Date: Fri, 5 May 2017 11:28:01 -0400 Subject: [PATCH 10/11] sub-process: move sub-process functions into separate files 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 Signed-off-by: Junio C Hamano --- Documentation/technical/api-sub-process.txt | 59 +++++++++++ Makefile | 1 + convert.c | 104 +------------------- sub-process.c | 102 +++++++++++++++++++ sub-process.h | 49 +++++++++ 5 files changed, 212 insertions(+), 103 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..793508cf3e --- /dev/null +++ b/Documentation/technical/api-sub-process.txt @@ -0,0 +1,59 @@ +sub-process API +=============== + +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. + +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 negotiate the interface version and capabilities. + + +Functions +--------- + +`cmd2process_cmp`:: + + Function to test two subprocess hashmap entries for equality. + +`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. + +`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=" + 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 cfae45aeee..79d04907a5 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,12 +497,6 @@ 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; @@ -510,65 +505,6 @@ struct cmd2process { 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(struct hashmap *hashmap, const char *cmd) -{ - struct subprocess_entry key; - - hashmap_entry_init(&key, strhash(cmd)); - key.cmd = cmd; - return hashmap_get(hashmap, &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=" 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 hashmap *hashmap, struct subprocess_entry *entry) -{ - if (!entry) - return; - - entry->process.clean_on_exit = 0; - kill(entry->process.pid, SIGTERM); - finish_command(&entry->process); - - hashmap_remove(hashmap, 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); -} - static int start_multi_file_filter_fn(struct subprocess_entry *subprocess) { int err; @@ -629,44 +565,6 @@ done: return err; } -typedef int(*subprocess_start_fn)(struct subprocess_entry *entry); -int subprocess_start(struct hashmap *hashmap, struct subprocess_entry *entry, const char *cmd, - subprocess_start_fn startfn) -{ - int err; - struct child_process *process; - const char *argv[] = { cmd, NULL }; - - 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(hashmap, entry); - return err; - } - - hashmap_add(hashmap, 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..536b60cced --- /dev/null +++ b/sub-process.c @@ -0,0 +1,102 @@ +/* + * Generic implementation of background process infrastructure. + */ +#include "sub-process.h" +#include "sigchain.h" +#include "pkt-line.h" + +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(struct hashmap *hashmap, const char *cmd) +{ + struct subprocess_entry key; + + hashmap_entry_init(&key, strhash(cmd)); + key.cmd = cmd; + return hashmap_get(hashmap, &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=" line wins */ + if (!strcmp(pair[0]->buf, "status=")) { + strbuf_reset(status); + strbuf_addbuf(status, pair[1]); + } + } + strbuf_list_free(pair); + } +} + +void subprocess_stop(struct hashmap *hashmap, struct subprocess_entry *entry) +{ + if (!entry) + return; + + entry->process.clean_on_exit = 0; + kill(entry->process.pid, SIGTERM); + finish_command(&entry->process); + + hashmap_remove(hashmap, 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 hashmap *hashmap, struct subprocess_entry *entry, const char *cmd, + subprocess_start_fn startfn) +{ + int err; + struct child_process *process; + const char *argv[] = { cmd, NULL }; + + 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(hashmap, entry); + return err; + } + + hashmap_add(hashmap, entry); + return 0; +} diff --git a/sub-process.h b/sub-process.h new file mode 100644 index 0000000000..a88e782bfc --- /dev/null +++ b/sub-process.h @@ -0,0 +1,49 @@ +#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 */ + +int cmd2process_cmp(const struct subprocess_entry *e1, + const struct subprocess_entry *e2, const void *unused); + +typedef int(*subprocess_start_fn)(struct subprocess_entry *entry); +int subprocess_start(struct hashmap *hashmap, struct subprocess_entry *entry, const char *cmd, + subprocess_start_fn startfn); + +void subprocess_stop(struct hashmap *hashmap, struct subprocess_entry *entry); + +struct subprocess_entry *subprocess_find_entry(struct hashmap *hashmap, 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=" + * key/value pairs and return the value from the last "status" packet + */ + +void subprocess_read_status(int fd, struct strbuf *status); + +#endif From 4f2a2e9f0e26c1c543d1f282d6e88b3d0f608d07 Mon Sep 17 00:00:00 2001 From: Ben Peart Date: Fri, 5 May 2017 11:28:02 -0400 Subject: [PATCH 11/11] convert: update subprocess_read_status() to not die on EOF Enable sub-processes to gracefully handle when the process dies by updating subprocess_read_status to return an error on EOF instead of dying. Update apply_multi_file_filter to take advantage of the revised subprocess_read_status. Signed-off-by: Ben Peart Signed-off-by: Junio C Hamano --- convert.c | 10 ++++++++-- sub-process.c | 10 +++++++--- sub-process.h | 2 +- 3 files changed, 16 insertions(+), 6 deletions(-) diff --git a/convert.c b/convert.c index 79d04907a5..f1e168bc30 100644 --- a/convert.c +++ b/convert.c @@ -635,7 +635,10 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len if (err) goto done; - subprocess_read_status(process->out, &filter_status); + err = subprocess_read_status(process->out, &filter_status); + if (err) + goto done; + err = strcmp(filter_status.buf, "success"); if (err) goto done; @@ -644,7 +647,10 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len if (err) goto done; - subprocess_read_status(process->out, &filter_status); + err = subprocess_read_status(process->out, &filter_status); + if (err) + goto done; + err = strcmp(filter_status.buf, "success"); done: diff --git a/sub-process.c b/sub-process.c index 536b60cced..92f8aea70a 100644 --- a/sub-process.c +++ b/sub-process.c @@ -21,13 +21,15 @@ struct subprocess_entry *subprocess_find_entry(struct hashmap *hashmap, const ch return hashmap_get(hashmap, &key, NULL); } -void subprocess_read_status(int fd, struct strbuf *status) +int subprocess_read_status(int fd, struct strbuf *status) { struct strbuf **pair; char *line; + int len; + for (;;) { - line = packet_read_line(fd, NULL); - if (!line) + len = packet_read_line_gently(fd, NULL, &line); + if ((len < 0) || !line) break; pair = strbuf_split_str(line, '=', 2); if (pair[0] && pair[0]->len && pair[1]) { @@ -39,6 +41,8 @@ void subprocess_read_status(int fd, struct strbuf *status) } strbuf_list_free(pair); } + + return (len < 0) ? len : 0; } void subprocess_stop(struct hashmap *hashmap, struct subprocess_entry *entry) diff --git a/sub-process.h b/sub-process.h index a88e782bfc..7d451e1cde 100644 --- a/sub-process.h +++ b/sub-process.h @@ -44,6 +44,6 @@ static inline struct child_process *subprocess_get_child_process( * key/value pairs and return the value from the last "status" packet */ -void subprocess_read_status(int fd, struct strbuf *status); +int subprocess_read_status(int fd, struct strbuf *status); #endif