Thread (23 messages) 23 messages, 3 authors, 2017-04-20

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

flat view

From: Lars Schneider <hidden>
Date: 2017-04-10 10:18:53

quoted hunk ↗ jump to hunk
On 07 Apr 2017, at 14:03, Ben Peart [off-list ref] 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 <redacted>
---
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.

{
-	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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help