Use stored commands for bindings

The preceding commit includes motivation; specifically,
invoking keyboard and mouse bindings should never fail
unless they perform an action which can fail. For instance,
a `bindcode Ctrl+9 exit` should always exit; of course,
`exec` may fail for external reasons.

The core changes are to bind.c and commands.c. As a single
command may actually, on being parsed, generate multiple
commands to be executed (consider `workspace 1; splith`),
the parsing code in execute_command is moved to a separate
function, parse_command, which returns a list_t of stored
commands; leaving execute_command to parse the command,
execute each resulting list element; and then cleanup.

An additional field for the list of parsed stored commands,
is added to struct sway_binding, usurping the name "command".
This is initialized via parse_command, and then
seat_execute_command is adjusted accordingly.

Side effects of this change include:
* Reference counting `struct criteria`, because a criterion
parsed once may apply to multiple stored commands, and
the alternative of duplicating criteria is even more
complicated.
* Adding reading and active arguments to config_handler,
because when bindsyms parse commands, they require handler
lookups to be performed as though it were runtime.
* Reference counting `struct sway_binding`, because the
alternative of updating sway_binding_dup is even more
complicated when allocation failures are be taken into
account.

common/list.c

@@ -18,6 +18,7 @@ list_t *create_list(void) {
 static void list_resize(list_t *list) {
 	if (list->length == list->capacity) {
 		list->capacity += 10;
+		// TODO: safely handle realloc failure
 		list->items = realloc(list->items, sizeof(void*) * list->capacity);
 	}
 }

include/sway/commands.h

@@ -47,7 +47,7 @@ struct cmd_results *checkarg(int argc, const char *name,
 		enum expected_args type, int val);
 
 struct cmd_handler *find_handler(char *line, struct cmd_handler *cmd_handlers,
-		int handlers_size);
+		int handlers_size, bool reading, bool active);
 /**
  * Parse and executes a command.
  */
@@ -56,10 +56,17 @@ struct cmd_results *execute_command(const char *command,  struct sway_seat *seat
 struct stored_command {
 	sway_cmd *handle;
 	int argc;
-	char** argv;
+	char **argv;
-	struct criteria* criteria;
+	struct criteria *criteria;
 };
+void free_stored_command(struct stored_command *command);
 
+/**
+ * Parses a command, and returns a list of stored_commands. If `for_runtime`
+ * is true, then command handler lookup is performed as though it were runtime;
+ * otherwise, as appropriate for the current `config` state.
+ */
+list_t *parse_command(const char *command, bool for_runtime);
 /**
  * Executes a pre-parsed command. (Returns NULL if command was not unsuccessful: TODO, fix)
  */

include/sway/config.h

@@ -38,7 +38,9 @@ enum binding_flags {
 };
 
 /**
- * A key binding and an associated command.
+ * A key binding and an associated command. In order to extend the lifetime
+ * of a binding slightly while executing the reload command, bindings are
+ * reference counted, and should only be freed if the reference count is <= 0.
  */
 struct sway_binding {
 	enum binding_input_type type;
@@ -46,15 +48,9 @@ struct sway_binding {
 	uint32_t flags;
 	list_t *keys; // sorted in ascending order
 	uint32_t modifiers;
-	char *command;
+	char *command_str; // original command string for IPC compatibility
-};
+	list_t *command; // a preparsed list of struct stored_command
-
+	int refcount;
-/**
- * A mouse binding and an associated command.
- */
-struct sway_mouse_binding {
-	uint32_t button;
-	char *command;
 };
 
 /**

include/sway/criteria.h

@@ -36,6 +36,9 @@ struct criteria {
 	bool tiling;
 	char urgent; // 'l' for latest or 'o' for oldest
 	char *workspace;
+	// optional refcount for users (like struct stored_command) for which
+	// a criterion may be referenced multiple times.
+	int refcount;
 };
 
 bool criteria_is_empty(struct criteria *criteria);

sway/commands.c

@@ -170,12 +170,12 @@ static int handler_compare(const void *_a, const void *_b) {
 }
 
 struct cmd_handler *find_handler(char *line, struct cmd_handler *cmd_handlers,
-		int handlers_size) {
+		int handlers_size, bool reading, bool active) {
 	struct cmd_handler d = { .command=line };
 	struct cmd_handler *res = NULL;
 	wlr_log(WLR_DEBUG, "find_handler(%s)", line);
 
-	bool config_loading = config->reading || !config->active;
+	bool config_loading = reading || !active;
 
 	if (!config_loading) {
 		res = bsearch(&d, command_handlers,
@@ -210,14 +210,11 @@ struct cmd_handler *find_handler(char *line, struct cmd_handler *cmd_handlers,
 }
 
 struct cmd_results *execute_command(const char *_exec, struct sway_seat *seat) {
-	// Even though this function will process multiple commands we will only
+	list_t *subcommands = parse_command(_exec, false);
-	// return the last error, if any (for now). (Since we have access to an
+	if (!subcommands) {
-	// error string we could e.g. concatenate all errors there.)
+		return cmd_results_new(CMD_INVALID, _exec,
-	struct cmd_results *results = NULL;
+				"Failed to allocate list of commands");
-	char *exec = strdup(_exec);
+	}
-	char *head = exec;
-	char *cmdlist;
-	char *cmd;
 
 	if (seat == NULL) {
 		// passing a NULL seat means we just pick the default seat
@@ -227,7 +224,53 @@ struct cmd_results *execute_command(const char *_exec, struct sway_seat *seat) {
 		}
 	}
 
-	head = exec;
+	// Even though this function will process multiple commands we will only
+	// return the last error, if any (for now). (Since we have access to an
+	// error string we could e.g. concatenate all errors there.)
+	struct cmd_results *results = NULL;
+	for (int i = 0; i < subcommands->length; i++) {
+		struct stored_command *command = subcommands->items[i];
+		struct cmd_results *res = execute_stored_command(command, seat);
+		if (res->status != CMD_SUCCESS) {
+			if (results) {
+				free_cmd_results(results);
+			}
+			results = res;
+			goto cleanup;
+		}
+	}
+cleanup:
+	for (int i = 0; i < subcommands->length; i++) {
+		free_stored_command(subcommands->items[i]);
+	}
+	list_free(subcommands);
+	if (!results) {
+		results = cmd_results_new(CMD_SUCCESS, NULL, NULL);
+	}
+	return results;
+}
+
+void free_stored_command(struct stored_command *command) {
+	free_argv(command->argc, command->argv);
+	if (command->criteria) {
+		command->criteria->refcount--;
+		if (command->criteria->refcount <= 0) {
+			criteria_destroy(command->criteria);
+		}
+	}
+	free(command);
+}
+
+list_t *parse_command(const char *_exec, bool for_runtime) {
+	char *exec = strdup(_exec);
+	if (!exec) {
+		return NULL;
+	}
+	list_t *stored_list = create_list();
+
+	bool abort = false;
+
+	char *head = exec;
 	do {
 		// Extract criteria (valid for this command list only).
 		struct criteria *criteria = NULL;
@@ -235,28 +278,28 @@ struct cmd_results *execute_command(const char *_exec, struct sway_seat *seat) {
 			char *error = NULL;
 			criteria = criteria_parse(head, &error);
 			if (!criteria) {
-				results = cmd_results_new(CMD_INVALID, head,
+				wlr_log(WLR_ERROR, "%s", error);
-					"%s", error);
 				free(error);
 				goto cleanup;
 			}
+			criteria->refcount++;
 			head += strlen(criteria->raw);
 			// Skip leading whitespace
 			head += strspn(head, whitespace);
 		}
 		// Split command list
-		cmdlist = argsep(&head, ";");
+		char *cmdlist = argsep(&head, ";");
 		cmdlist += strspn(cmdlist, whitespace);
 		do {
 			// Split commands
-			cmd = argsep(&cmdlist, ",");
+			char *cmd = argsep(&cmdlist, ",");
 			cmd += strspn(cmd, whitespace);
 			if (strcmp(cmd, "") == 0) {
 				wlr_log(WLR_INFO, "Ignoring empty command.");
 				continue;
 			}
 			wlr_log(WLR_INFO, "Handling command '%s'", cmd);
-			//TODO better handling of argv
+			// TODO better handling of argv
 			int argc;
 			char **argv = split_args(cmd, &argc);
 			if (strcmp(argv[0], "exec") != 0) {
@@ -267,17 +310,20 @@ struct cmd_results *execute_command(const char *_exec, struct sway_seat *seat) {
 					}
 				}
 			}
-			struct cmd_handler *handler = find_handler(argv[0], NULL, 0);
+			// Identify the command
+			struct cmd_handler *handler;
+			if (for_runtime) {
+				handler = find_handler(argv[0], NULL, 0, false, true);
+			} else {
+				handler = find_handler(argv[0], NULL, 0,
+						config->reading, config->active);
+			}
+
 			if (!handler) {
-				if (results) {
+				wlr_log(WLR_ERROR, "Unknown/invalid command");
-					free_cmd_results(results);
-				}
-				results = cmd_results_new(CMD_INVALID, cmd, "Unknown/invalid command");
 				free_argv(argc, argv);
-				if (criteria) {
+				abort = true;
-					criteria_destroy(criteria);
+				break;
-				}
-				goto cleanup;
 			}
 
 			// Var replacement, for all but first argument of set
@@ -286,38 +332,51 @@ struct cmd_results *execute_command(const char *_exec, struct sway_seat *seat) {
 				unescape_string(argv[i]);
 			}
 
-			struct stored_command command;
+			struct stored_command *command = (struct stored_command *)
-			command.handle = handler->handle;
+					calloc(1, sizeof(struct stored_command));
-			command.argv = argv;
+			if (!command) {
-			command.argc = argc;
+				wlr_log(WLR_ERROR, "Unknown/invalid command");
-			command.criteria = criteria;
+				abort = true;
-			struct cmd_results *res = execute_stored_command(&command, seat);
+				break;
-			free_argv(argc, argv);
-
-			if (res->status != CMD_SUCCESS) {
-				if (results) {
-					free_cmd_results(results);
-				}
-				results = res;
-				if (criteria) {
-					criteria_destroy(criteria);
-				}
-				goto cleanup;
 			}
-			free_cmd_results(res);
 
+			// TODO: handle realloc failure case for list_add
+			list_add(stored_list, command);
+
+			command->handle = handler->handle;
+			command->argv = argv;
+			command->argc = argc;
+			command->criteria = criteria;
+			if (criteria) {
+				command->criteria->refcount++;
+			}
 		} while(cmdlist);
+
+		if (criteria) {
+			criteria->refcount--;
+			if (criteria->refcount <= 0) {
+				criteria_destroy(criteria);
+			}
+		}
+		if (abort) {
+			break;
+		}
 	} while(head);
+
 cleanup:
 	free(exec);
-	if (!results) {
+	return stored_list;
-		results = cmd_results_new(CMD_SUCCESS, NULL, NULL);
-	}
-	return results;
 }
 
-struct cmd_results *execute_stored_command(const struct stored_command* command,
+struct cmd_results *execute_stored_command(const struct stored_command *command,
-		struct sway_seat* seat) {
+		struct sway_seat *seat) {
+	if (seat == NULL) {
+		// passing a NULL seat means we just pick the default seat
+		seat = input_manager_get_default_seat(input_manager);
+		if (!sway_assert(seat, "could not find a seat to run the command on")) {
+			return NULL;
+		}
+	}
 	config->handler_context.seat = seat;
 
 	if (!command->criteria) {
@@ -378,7 +437,8 @@ struct cmd_results *config_command(char *exec) {
 		goto cleanup;
 	}
 	wlr_log(WLR_INFO, "handling config command '%s'", exec);
-	struct cmd_handler *handler = find_handler(argv[0], NULL, 0);
+	struct cmd_handler *handler = find_handler(argv[0], NULL, 0,
+			config->reading, config->active);
 	if (!handler) {
 		char *input = argv[0] ? argv[0] : "(empty)";
 		results = cmd_results_new(CMD_INVALID, input, "Unknown/invalid command");
@@ -414,7 +474,7 @@ struct cmd_results *config_subcommand(char **argv, int argc,
 	free(command);
 
 	struct cmd_handler *handler = find_handler(argv[0], handlers,
-			handlers_size);
+			handlers_size, config->reading, config->active);
 	if (!handler) {
 		char *input = argv[0] ? argv[0] : "(empty)";
 		return cmd_results_new(CMD_INVALID, input, "Unknown/invalid command");
@@ -448,7 +508,8 @@ struct cmd_results *config_commands_command(char *exec) {
 		goto cleanup;
 	}
 
-	struct cmd_handler *handler = find_handler(cmd, NULL, 0);
+	struct cmd_handler *handler = find_handler(cmd, NULL, 0,
+			config->reading, config->active);
 	if (!handler && strcmp(cmd, "*") != 0) {
 		results = cmd_results_new(CMD_INVALID, cmd, "Unknown/invalid command");
 		goto cleanup;

sway/commands/bar.c

@@ -48,7 +48,8 @@ struct cmd_results *cmd_bar(int argc, char **argv) {
 
 	if (!config->reading) {
 		if (!find_handler(argv[0], bar_config_handlers,
-					sizeof(bar_config_handlers))) {
+				sizeof(bar_config_handlers), config->reading,
+				config->active)) {
 			return cmd_results_new(CMD_FAILURE, "bar",
 					"Can only be used in config file.");
 		}
@@ -58,8 +59,10 @@ struct cmd_results *cmd_bar(int argc, char **argv) {
 
 	if (argc > 1) {
 		struct bar_config *bar = NULL;
-		if (!find_handler(argv[0], bar_handlers, sizeof(bar_handlers))
+		if (!find_handler(argv[0], bar_handlers, sizeof(bar_handlers),
-				&& find_handler(argv[1], bar_handlers, sizeof(bar_handlers))) {
+				  config->reading, config->active)
+				&& find_handler(argv[1], bar_handlers, sizeof(bar_handlers),
+						config->reading, config->active)) {
 			for (int i = 0; i < config->bars->length; ++i) {
 				struct bar_config *item = config->bars->items[i];
 				if (strcmp(item->id, argv[0]) == 0) {

sway/commands/bind.c

@@ -20,43 +20,21 @@ int binding_order = 0;
 
 void free_sway_binding(struct sway_binding *binding) {
 	if (!binding) {
+		wlr_log(WLR_DEBUG, "Unexpected: tried to free a null binding");
 		return;
 	}
 
 	if (binding->keys) {
 		free_flat_list(binding->keys);
 	}
-	free(binding->command);
+	for (int i = 0; i < binding->command->length; i++) {
+		free_stored_command(binding->command->items[i]);
+	}
+	list_free(binding->command);
+	free(binding->command_str);
 	free(binding);
 }
 
-static struct sway_binding *sway_binding_dup(struct sway_binding *sb) {
-	struct sway_binding *new_sb = calloc(1, sizeof(struct sway_binding));
-	if (!new_sb) {
-		return NULL;
-	}
-
-	new_sb->type = sb->type;
-	new_sb->order = sb->order;
-	new_sb->flags = sb->flags;
-	new_sb->modifiers = sb->modifiers;
-	new_sb->command = strdup(sb->command);
-
-	new_sb->keys = create_list();
-	int i;
-	for (i = 0; i < sb->keys->length; ++i) {
-		xkb_keysym_t *key = malloc(sizeof(xkb_keysym_t));
-		if (!key) {
-			free_sway_binding(new_sb);
-			return NULL;
-		}
-		*key = *(xkb_keysym_t *)sb->keys->items[i];
-		list_add(new_sb->keys, key);
-	}
-
-	return new_sb;
-}
-
 /**
  * Returns true if the bindings have the same key and modifier combinations.
  * Note that keyboard layout is not considered, so the bindings might actually
@@ -110,8 +88,8 @@ static int key_qsort_cmp(const void *keyp_a, const void *keyp_b) {
  * the value of *type if the initial type guess was incorrect and if this
  * was the first identified key.
  */
-static struct cmd_results *identify_key(const char* name, bool first_key,
+static struct cmd_results *identify_key(const char *name, bool first_key,
-		uint32_t* key_val, enum binding_input_type* type) {
+		uint32_t *key_val, enum binding_input_type *type) {
 	if (*type == BINDING_KEYCODE) {
 		// check for keycode
 		xkb_keycode_t keycode = strtol(name, NULL, 10);
@@ -180,6 +158,7 @@ static struct cmd_results *cmd_bindsym_or_bindcode(int argc, char **argv,
 	binding->modifiers = 0;
 	binding->flags = 0;
 	binding->type = bindcode ? BINDING_KEYCODE : BINDING_KEYSYM;
+	binding->refcount = 1;
 
 	bool exclude_titlebar = false;
 
@@ -213,7 +192,18 @@ static struct cmd_results *cmd_bindsym_or_bindcode(int argc, char **argv,
 			"(expected at least 2 non-option arguments, got %d)", bindtype, argc);
 	}
 
-	binding->command = join_args(argv + 1, argc - 1);
+	binding->command_str = join_args(argv + 1, argc - 1);
+	if (!binding->command_str) {
+		free_sway_binding(binding);
+		return cmd_results_new(CMD_FAILURE, bindtype, "Unable to allocate command string");
+	}
+	binding->command = parse_command(binding->command_str, true);
+	if (!binding->command) {
+		free_sway_binding(binding);
+		return cmd_results_new(CMD_FAILURE, bindtype,
+				"Unable to allocate list of commands");
+	}
+
 
 	list_t *split = split_string(argv[0], "+");
 	for (int i = 0; i < split->length; ++i) {
@@ -272,8 +262,9 @@ static struct cmd_results *cmd_bindsym_or_bindcode(int argc, char **argv,
 	for (int i = 0; i < mode_bindings->length; ++i) {
 		struct sway_binding *config_binding = mode_bindings->items[i];
 		if (binding_key_compare(binding, config_binding)) {
-			wlr_log(WLR_DEBUG, "overwriting old binding with command '%s'",
+			wlr_log(WLR_DEBUG, "overwriting old binding, #%d with command %s",
-				config_binding->command);
+					config_binding->order,
+					config_binding->command_str);
 			free_sway_binding(config_binding);
 			mode_bindings->items[i] = binding;
 			overwritten = true;
@@ -284,8 +275,8 @@ static struct cmd_results *cmd_bindsym_or_bindcode(int argc, char **argv,
 		list_add(mode_bindings, binding);
 	}
 
-	wlr_log(WLR_DEBUG, "%s - Bound %s to command %s",
+	wlr_log(WLR_DEBUG, "%s - Binding #%d bound %s with command '%s'",
-		bindtype, argv[0], binding->command);
+		bindtype,  binding->order, argv[0], binding->command_str);
 	return cmd_results_new(CMD_SUCCESS, NULL, NULL);
 
 }
@@ -303,33 +294,38 @@ struct cmd_results *cmd_bindcode(int argc, char **argv) {
  * Execute the command associated to a binding
  */
 void seat_execute_command(struct sway_seat *seat, struct sway_binding *binding) {
-	wlr_log(WLR_DEBUG, "running command for binding: %s",
+	for (int i = 0; i < binding->command->length; i++) {
-		binding->command);
+		struct stored_command *command = binding->command->items[i];
 
-	struct sway_binding *binding_copy = binding;
+		wlr_log(WLR_DEBUG, "running command for binding, '%s' (with %d arguments)",
-	bool reload = false;
+				command->argv[0], command->argc - 1);
-	// if this is a reload command we need to make a duplicate of the
+
-	// binding since it will be gone after the reload has completed.
+		bool reload = false;
-	if (strcasecmp(binding->command, "reload") == 0) {
+		// if this is a reload command, we increment the refcount to
-		reload = true;
+		// keep the binding alive up to the end of this function
-		binding_copy = sway_binding_dup(binding);
+		if (command->handle == cmd_reload) {
-		if (!binding_copy) {
+			reload = true;
-			wlr_log(WLR_ERROR, "Failed to duplicate binding during reload");
+			binding->refcount++;
+		}
+
+		config->handler_context.seat = seat;
+		struct cmd_results *results = execute_stored_command(command, NULL);
+		if (results->status == CMD_SUCCESS) {
+			ipc_event_binding(binding);
+		} else {
+			wlr_log(WLR_DEBUG, "could not run command for binding #%d: %s",
+				binding->order, results->error);
+		}
+		free_cmd_results(results);
+
+		if (reload) {
+			// free the binding if/once it's no longer referenced,
+			// and return, since this binding may have been freed
+			binding->refcount--;
+			if (binding->refcount <= 0) {
+				free_sway_binding(binding);
+			}
 			return;
 		}
 	}
-
-	config->handler_context.seat = seat;
-	struct cmd_results *results = execute_command(binding->command, NULL);
-	if (results->status == CMD_SUCCESS) {
-		ipc_event_binding(binding_copy);
-	} else {
-		wlr_log(WLR_DEBUG, "could not run command for binding: %s (%s)",
-			binding->command, results->error);
-	}
-
-	if (reload) { // free the binding if we made a copy
-		free_sway_binding(binding_copy);
-	}
-	free_cmd_results(results);
 }

sway/commands/input.c

@@ -53,7 +53,8 @@ struct cmd_results *cmd_input(int argc, char **argv) {
 	struct cmd_results *res;
 
 	if (find_handler(argv[1], input_config_handlers,
-			sizeof(input_config_handlers))) {
+			sizeof(input_config_handlers), config->reading,
+			config->active)) {
 		if (config->reading) {
 			res = config_subcommand(argv + 1, argc - 1,
 				input_config_handlers, sizeof(input_config_handlers));

sway/commands/output.c

@@ -40,7 +40,8 @@ struct cmd_results *cmd_output(int argc, char **argv) {
 		config->handler_context.leftovers.argc = 0;
 		config->handler_context.leftovers.argv = NULL;
 
-		if (find_handler(*argv, output_handlers, sizeof(output_handlers))) {
+		if (find_handler(*argv, output_handlers, sizeof(output_handlers),
+				config->reading, config->active)) {
 			error = config_subcommand(argv, argc, output_handlers,
 					sizeof(output_handlers));
 		} else {

sway/config.c

@@ -47,19 +47,31 @@ static void free_mode(struct sway_mode *mode) {
 	free(mode->name);
 	if (mode->keysym_bindings) {
 		for (i = 0; i < mode->keysym_bindings->length; i++) {
-			free_sway_binding(mode->keysym_bindings->items[i]);
+			struct sway_binding* binding = mode->keysym_bindings->items[i];
+			binding->refcount--;
+			if (binding->refcount <= 0) {
+				free_sway_binding(binding);
+			}
 		}
 		list_free(mode->keysym_bindings);
 	}
 	if (mode->keycode_bindings) {
 		for (i = 0; i < mode->keycode_bindings->length; i++) {
-			free_sway_binding(mode->keycode_bindings->items[i]);
+			struct sway_binding* binding = mode->keycode_bindings->items[i];
+			binding->refcount--;
+			if (binding->refcount <= 0) {
+				free_sway_binding(binding);
+			}
 		}
 		list_free(mode->keycode_bindings);
 	}
 	if (mode->mouse_bindings) {
 		for (i = 0; i < mode->mouse_bindings->length; i++) {
-			free_sway_binding(mode->mouse_bindings->items[i]);
+			struct sway_binding* binding = mode->mouse_bindings->items[i];
+			binding->refcount--;
+			if (binding->refcount <= 0) {
+				free_sway_binding(binding);
+			}
 		}
 		list_free(mode->mouse_bindings);
 	}

sway/criteria.c

@@ -525,6 +525,7 @@ struct criteria *criteria_parse(char *raw, char **error_arg) {
 	}
 	++head;
 
+	// zero allocation also ensures the optional refcount defaults to zero
 	struct criteria *criteria = calloc(sizeof(struct criteria), 1);
 	char *name = NULL, *value = NULL;
 	bool in_quotes = false;

sway/ipc-server.c

@@ -379,7 +379,7 @@ void ipc_event_binding(struct sway_binding *binding) {
 	wlr_log(WLR_DEBUG, "Sending binding event");
 
 	json_object *json_binding = json_object_new_object();
-	json_object_object_add(json_binding, "command", json_object_new_string(binding->command));
+	json_object_object_add(json_binding, "command", json_object_new_string(binding->command_str));
 
 	const char *names[10];
 	int len = get_modifier_names(names, binding->modifiers);

sway/tree/workspace.c

@@ -162,7 +162,7 @@ static bool workspace_valid_on_output(const char *output_name,
 
 static void workspace_name_from_binding(const struct sway_binding * binding,
 		const char* output_name, int *min_order, char **earliest_name) {
-	char *cmdlist = strdup(binding->command);
+	char *cmdlist = strdup(binding->command_str);
 	char *dup = cmdlist;
 	char *name = NULL;