git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v5 4/8] convert: Separate generic structures and variables from the filter specific ones

From
Lars Schneider <larsxschneider@gmail.com>
Date
Apr 10, 2017, 10:18 UTC
Message-ID
<48FA4601-0819-4DE2-943A-7A791BA7C583@gmail.com>
In-Reply-To
<20170407120354.17736-5-benpeart@microsoft.com>
Show 52 quoted lines
> On 07 Apr 2017, at 14:03, Ben Peart <peartben@gmail.com> wrote:
> 
> To enable future reuse of the filter.<driver>.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. Also move
> all knowledge of the hashmap into the generic functions.
> 
> cmd2process is a filter protocol specific structure that is used to
> track the negotiated capabilities of the filter.
> 
> Signed-off-by: Ben Peart <benpeart@microsoft.com>
> ---
> convert.c | 57 +++++++++++++++++++++++++++++++--------------------------
> 1 file changed, 31 insertions(+), 26 deletions(-)
> 
> diff --git a/convert.c b/convert.c
> index 404757eac9..f569026511 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -496,29 +496,40 @@ 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(const char *cmd)

I am curious why you removed the hashmap parameter (here and in other pars of this patch). I know the parameter is not strictly necessary as the hashmap is a global variable anyways. However, I think it eases code maintainability in the long run if a function is "as pure as possible" (IOW does rely on global state as less as possible).

As I consider this personal preference I think either way is fine.
Show 10 quoted lines
> {
> -	struct cmd2process key;
> +	struct subprocess_entry key;
> +
> +	if (!cmd_process_map_initialized) {
> +		cmd_process_map_initialized = 1;
> +		hashmap_init(&cmd_process_map, (hashmap_cmp_fn)cmd2process_cmp, 0);
> +		return NULL;
> +	}
> +

I am a bit in doubt about this one. If the process map was not initialized yet, then I would expect "find_multi_file_filter_entry" to return NULL. Creating the hash map as side effect seems a bit unexpected to me.

The rest of the patch looks good to me.

Thanks, Lars

Previous: Ben PeartNext: Junio C Hamano
Message 21 of 27 in “refactor the filter process code into a reusable module”
  1. 0/8 refactor the filter process code into a reusable moduleBen Peart, Apr 7, 2017
  2. 1/8 pkt-line: add packet_read_line_gently()Ben Peart, Apr 7, 2017
  3. Lars SchneiderApr 9, 2017
  4. 2/8 convert: move packet_write_list() into pkt-line as packet_writel()Ben Peart, Apr 7, 2017
  5. Lars SchneiderApr 9, 2017
  6. 3/8 convert: Split start_multi_file_filter into two separate functionsBen Peart, Apr 7, 2017
  7. Lars SchneiderApr 9, 2017
  8. Jeff KingApr 11, 2017
  9. Lars SchneiderApr 11, 2017
  10. Jeff KingApr 11, 2017
  11. Lars SchneiderApr 11, 2017
  12. Jeff KingApr 11, 2017
  13. Ben PeartApr 20, 2017
  14. 6/8 convert: rename reusable sub-process functionsBen Peart, Apr 7, 2017
  15. Lars SchneiderApr 10, 2017
  16. 7/8 sub-process: move sub-process functions into separate filesBen Peart, Apr 7, 2017
  17. Lars SchneiderApr 10, 2017
  18. 8/8 convert: Update subprocess_read_status to not die on EOFBen Peart, Apr 7, 2017
  19. Lars SchneiderApr 10, 2017
  20. 4/8 convert: Separate generic structures and variables from the filter specific onesBen Peart, Apr 7, 2017
  21. Lars SchneiderApr 10, 2017
  22. Junio C HamanoApr 17, 2017
  23. Ben PeartApr 18, 2017
  24. Junio C HamanoApr 19, 2017
  25. Ben PeartApr 20, 2017
  26. 5/8 convert: Update generic functions to only use generic data structuresBen Peart, Apr 7, 2017
  27. Lars SchneiderApr 10, 2017

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.