From: Lars Schneider <larsxschneider@gmail.com>
To: Ben Peart <peartben@gmail.com>
Cc: git@vger.kernel.org, gitster@pobox.com, benpeart@microsoft.com,
christian.couder@gmail.com
Subject: Re: [PATCH v5 7/8] sub-process: move sub-process functions into separate files
Date: Mon, 10 Apr 2017 14:41:52 +0200 [thread overview]
Message-ID: <40AA3048-3B05-4950-8981-942DB58B464F@gmail.com> (raw)
In-Reply-To: <20170407120354.17736-8-benpeart@microsoft.com>
> On 07 Apr 2017, at 14:03, Ben Peart <peartben@gmail.com> 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@microsoft.com>
> ---
> 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@gmail.com/
> +
> +`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@gmail.com/
TBH, I don't see why this fixes the test, though.
Thanks,
Lars
next prev parent reply other threads:[~2017-04-10 12:41 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-04-07 12:03 [PATCH v5 0/8] refactor the filter process code into a reusable module Ben Peart
2017-04-07 12:03 ` [PATCH v5 1/8] pkt-line: add packet_read_line_gently() Ben Peart
2017-04-09 19:34 ` Lars Schneider
2017-04-07 12:03 ` [PATCH v5 2/8] convert: move packet_write_list() into pkt-line as packet_writel() Ben Peart
2017-04-09 19:43 ` Lars Schneider
2017-04-07 12:03 ` [PATCH v5 3/8] convert: Split start_multi_file_filter into two separate functions Ben Peart
2017-04-09 19:56 ` Lars Schneider
2017-04-11 16:16 ` Jeff King
2017-04-11 19:29 ` Lars Schneider
2017-04-11 19:37 ` Jeff King
2017-04-11 20:01 ` Lars Schneider
2017-04-11 20:05 ` Jeff King
2017-04-20 17:27 ` Ben Peart
2017-04-07 12:03 ` [PATCH v5 4/8] convert: Separate generic structures and variables from the filter specific ones Ben Peart
2017-04-10 10:18 ` Lars Schneider
2017-04-17 3:31 ` Junio C Hamano
2017-04-18 16:38 ` Ben Peart
2017-04-19 1:23 ` Junio C Hamano
2017-04-20 17:24 ` Ben Peart
2017-04-07 12:03 ` [PATCH v5 5/8] convert: Update generic functions to only use generic data structures Ben Peart
2017-04-10 12:05 ` Lars Schneider
2017-04-07 12:03 ` [PATCH v5 6/8] convert: rename reusable sub-process functions Ben Peart
2017-04-10 12:11 ` Lars Schneider
2017-04-07 12:03 ` [PATCH v5 7/8] sub-process: move sub-process functions into separate files Ben Peart
2017-04-10 12:41 ` Lars Schneider [this message]
2017-04-07 12:03 ` [PATCH v5 8/8] convert: Update subprocess_read_status to not die on EOF Ben Peart
2017-04-10 12:48 ` Lars Schneider
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=40AA3048-3B05-4950-8981-942DB58B464F@gmail.com \
--to=larsxschneider@gmail.com \
--cc=benpeart@microsoft.com \
--cc=christian.couder@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=peartben@gmail.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).