From: Lars Schneider Date: Mon, 10 Apr 2017 12:05:52 GMT Subject: Re: [PATCH v5 5/8] convert: Update generic functions to only use generic data structures Message-ID: <5900F7F1-89D9-433E-A6C3-0AB27C815BE6@gmail.com> In-Reply-To: <20170407120354.17736-6-benpeart@microsoft.com> > On 07 Apr 2017, at 14:03, Ben Peart wrote: > > 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 > --- > convert.c | 46 ++++++++++++++++++++++++++++------------------ > 1 file changed, 28 insertions(+), 18 deletions(-) > > diff --git a/convert.c b/convert.c > index f569026511..747c0c363b 100644 > --- a/convert.c > +++ b/convert.c > @@ -576,14 +576,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; I wonder if a "subprocess_entry" should rather have a generic data pointer for any extra information. However, this is probably bikeshedding and I think this solution is equally good. > 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); > > @@ -638,17 +639,21 @@ static int start_multi_file_filter_fn(struct cmd2process *entry) > return err; > } > ... > static int apply_multi_file_filter(const char *path, const char *src, size_t len, > @@ -692,9 +698,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); > - if (!entry) > + entry = xmalloc(sizeof(*entry)); > + entry->supported_capabilities = 0; If we would use a generic data pointer then we could initialize supported_capabilities in "start_multi_file_filter_fn". Apart from the bike shedding above this patch looks good to me. Minor nit: It breaks t0021 "invalid process filter must fail (and not hang!)" test case but I assume this is corrected in a later patch. Cheers, Lars