From 1267e47de913d2cda2644ad89bba4e9c55842cd3 Mon Sep 17 00:00:00 2001 From: Kenny Levinsen Date: Sat, 16 Mar 2024 17:55:20 +0100 Subject: [PATCH 1/3] config/output: Refactor handling of tiered configs Output configuration can be applied to a particular output in three ways: As a wildcard, by connector name and by identifier. This in turn means that three different configurations must be handled at any given time. In the current model, this is managed by merging new configuration into every other matching configuration. At the same time, an additional synthetic configuration is made which matchehes both identifier and name at the same time, further complicating logic. Instead, manage and store each configuration independently and merge them in order when retrieving configuration for an output. When changes are made to a less specific configuration, clear these fields from more specific configurations to allow the change to take effect regardless of precedence. Fixes: https://github.com/swaywm/sway/issues/8048 --- include/sway/config.h | 9 +- sway/config/output.c | 294 +++++++++++++++++++----------------------- 2 files changed, 142 insertions(+), 161 deletions(-) diff --git a/include/sway/config.h b/include/sway/config.h index 40710199a..0be1cd229 100644 --- a/include/sway/config.h +++ b/include/sway/config.h @@ -693,7 +693,14 @@ bool apply_output_configs(struct matched_output_config *configs, void apply_all_output_configs(void); -struct output_config *store_output_config(struct output_config *oc); +/** + * store_output_config stores a new output config. An output may be matched by + * three different config types, in order of precedence: Identifier, name and + * wildcard. When storing a config type of lower precedence, assume that the + * user wants the config to take immediate effect by superseding (clearing) the + * same values from higher presedence configuration. + */ +void store_output_config(struct output_config *oc); struct output_config *find_output_config(struct sway_output *output); diff --git a/sway/config/output.c b/sway/config/output.c index 3f1c3126b..e5ff240ae 100644 --- a/sway/config/output.c +++ b/sway/config/output.c @@ -79,6 +79,71 @@ struct output_config *new_output_config(const char *name) { return oc; } +// supersede_output_config clears all fields in dst that were set in src +static void supersede_output_config(struct output_config *dst, struct output_config *src) { + if (src->enabled != -1) { + dst->enabled = -1; + } + if (src->width != -1) { + dst->width = -1; + } + if (src->height != -1) { + dst->height = -1; + } + if (src->x != -1) { + dst->x = -1; + } + if (src->y != -1) { + dst->y = -1; + } + if (src->scale != -1) { + dst->scale = -1; + } + if (src->scale_filter != SCALE_FILTER_DEFAULT) { + dst->scale_filter = SCALE_FILTER_DEFAULT; + } + if (src->subpixel != WL_OUTPUT_SUBPIXEL_UNKNOWN) { + dst->subpixel = WL_OUTPUT_SUBPIXEL_UNKNOWN; + } + if (src->refresh_rate != -1) { + dst->refresh_rate = -1; + } + if (src->custom_mode != -1) { + dst->custom_mode = -1; + } + if (src->drm_mode.type != (uint32_t) -1) { + dst->drm_mode.type = -1; + } + if (src->transform != -1) { + dst->transform = -1; + } + if (src->max_render_time != -1) { + dst->max_render_time = -1; + } + if (src->adaptive_sync != -1) { + dst->adaptive_sync = -1; + } + if (src->render_bit_depth != RENDER_BIT_DEPTH_DEFAULT) { + dst->render_bit_depth = RENDER_BIT_DEPTH_DEFAULT; + } + if (src->background) { + free(dst->background); + dst->background = NULL; + } + if (src->background_option) { + free(dst->background_option); + dst->background_option = NULL; + } + if (src->background_fallback) { + free(dst->background_fallback); + dst->background_fallback = NULL; + } + if (src->power != -1) { + dst->power = -1; + } +} + +// merge_output_config sets all fields in dst that were set in src static void merge_output_config(struct output_config *dst, struct output_config *src) { if (src->enabled != -1) { dst->enabled = src->enabled; @@ -142,96 +207,46 @@ static void merge_output_config(struct output_config *dst, struct output_config } } -static void merge_wildcard_on_all(struct output_config *wildcard) { - for (int i = 0; i < config->output_configs->length; i++) { - struct output_config *oc = config->output_configs->items[i]; - if (strcmp(wildcard->name, oc->name) != 0) { - sway_log(SWAY_DEBUG, "Merging output * config on %s", oc->name); - merge_output_config(oc, wildcard); - } - } -} - -static void merge_id_on_name(struct output_config *oc) { - struct sway_output *output = all_output_by_name_or_id(oc->name); - if (output == NULL) { - return; +void store_output_config(struct output_config *oc) { + bool merged = false; + bool wildcard = strcmp(oc->name, "*") == 0; + struct sway_output *output = wildcard ? NULL : output_by_name_or_id(oc->name); + if (!output && !wildcard) { + // There is no config by this name, just add it in + goto done; } - const char *name = output->wlr_output->name; char id[128]; output_get_identifier(id, sizeof(id), output); + for (int i = 0; i < config->output_configs->length; i++) { + struct output_config *old = config->output_configs->items[i]; - char *id_on_name = format_str("%s on %s", id, name); - if (!id_on_name) { - return; - } + // If the old config matches the new config's name, regardless of + // whether it was name or identifier, merge on top of the existing + // config. If the new config is a wildcard, this also merges on top of + // old wildcard configs. + if (strcmp(old->name, oc->name) == 0) { + merge_output_config(old, oc); + merged = true; + continue; + } - int i = list_seq_find(config->output_configs, output_name_cmp, id_on_name); - if (i >= 0) { - sway_log(SWAY_DEBUG, "Merging on top of existing id on name config"); - merge_output_config(config->output_configs->items[i], oc); - } else { - // If both a name and identifier config, exist generate an id on name - int ni = list_seq_find(config->output_configs, output_name_cmp, name); - int ii = list_seq_find(config->output_configs, output_name_cmp, id); - if ((ni >= 0 && ii >= 0) || (ni >= 0 && strcmp(oc->name, id) == 0) - || (ii >= 0 && strcmp(oc->name, name) == 0)) { - struct output_config *ion_oc = new_output_config(id_on_name); - if (ni >= 0) { - merge_output_config(ion_oc, config->output_configs->items[ni]); - } - if (ii >= 0) { - merge_output_config(ion_oc, config->output_configs->items[ii]); - } - merge_output_config(ion_oc, oc); - list_add(config->output_configs, ion_oc); - sway_log(SWAY_DEBUG, "Generated id on name output config \"%s\"" - " (enabled: %d) (%dx%d@%fHz position %d,%d scale %f " - "transform %d) (bg %s %s) (power %d) (max render time: %d)", - ion_oc->name, ion_oc->enabled, ion_oc->width, ion_oc->height, - ion_oc->refresh_rate, ion_oc->x, ion_oc->y, ion_oc->scale, - ion_oc->transform, ion_oc->background, - ion_oc->background_option, ion_oc->power, - ion_oc->max_render_time); + // If the new config is a wildcard config we supersede all non-wildcard + // configs. Old wildcard configs have already been handled above. + if (wildcard) { + supersede_output_config(old, oc); + continue; + } + + // If the new config matches an output's name, and the old config + // matches on that output's identifier, supersede it. + if (strcmp(old->name, id) == 0 && + strcmp(oc->name, output->wlr_output->name) == 0) { + supersede_output_config(old, oc); } } - free(id_on_name); -} - -struct output_config *store_output_config(struct output_config *oc) { - bool wildcard = strcmp(oc->name, "*") == 0; - if (wildcard) { - merge_wildcard_on_all(oc); - } else { - merge_id_on_name(oc); - } - - int i = list_seq_find(config->output_configs, output_name_cmp, oc->name); - if (i >= 0) { - sway_log(SWAY_DEBUG, "Merging on top of existing output config"); - struct output_config *current = config->output_configs->items[i]; - merge_output_config(current, oc); - free_output_config(oc); - oc = current; - } else if (!wildcard) { - sway_log(SWAY_DEBUG, "Adding non-wildcard output config"); - i = list_seq_find(config->output_configs, output_name_cmp, "*"); - if (i >= 0) { - sway_log(SWAY_DEBUG, "Merging on top of output * config"); - struct output_config *current = new_output_config(oc->name); - merge_output_config(current, config->output_configs->items[i]); - merge_output_config(current, oc); - free_output_config(oc); - oc = current; - } - list_add(config->output_configs, oc); - } else { - // New wildcard config. Just add it - sway_log(SWAY_DEBUG, "Adding output * config"); - list_add(config->output_configs, oc); - } +done: sway_log(SWAY_DEBUG, "Config stored for output %s (enabled: %d) (%dx%d@%fHz " "position %d,%d scale %f subpixel %s transform %d) (bg %s %s) (power %d) " "(max render time: %d)", @@ -240,7 +255,13 @@ struct output_config *store_output_config(struct output_config *oc) { oc->transform, oc->background, oc->background_option, oc->power, oc->max_render_time); - return oc; + // If the configuration was not merged into an existing configuration, add + // it to the list. Otherwise we're done with it and can free it. + if (!merged) { + list_add(config->output_configs, oc); + } else { + free_output_config(oc); + } } static void set_mode(struct wlr_output *output, struct wlr_output_state *pending, @@ -587,94 +608,47 @@ static void default_output_config(struct output_config *oc, oc->max_render_time = 0; } -static struct output_config *get_output_config(char *identifier, - struct sway_output *sway_output) { +// find_output_config returns a merged output_config containing all stored +// configuration that applies to the specified output. +struct output_config *find_output_config(struct sway_output *sway_output) { const char *name = sway_output->wlr_output->name; + struct output_config *oc = NULL; - struct output_config *oc_id_on_name = NULL; - struct output_config *oc_name = NULL; - struct output_config *oc_id = NULL; - - char *id_on_name = format_str("%s on %s", identifier, name); - int i = list_seq_find(config->output_configs, output_name_cmp, id_on_name); - if (i >= 0) { - oc_id_on_name = config->output_configs->items[i]; - } else { - i = list_seq_find(config->output_configs, output_name_cmp, name); - if (i >= 0) { - oc_name = config->output_configs->items[i]; - } - - i = list_seq_find(config->output_configs, output_name_cmp, identifier); - if (i >= 0) { - oc_id = config->output_configs->items[i]; - } - } - - struct output_config *result = new_output_config("temp"); + struct output_config *result = new_output_config(name); if (config->reloading) { default_output_config(result, sway_output->wlr_output); } - if (oc_id_on_name) { - // Already have an identifier on name config, use that - free(result->name); - result->name = strdup(id_on_name); - merge_output_config(result, oc_id_on_name); - } else if (oc_name && oc_id) { - // Generate a config named ` on ` which contains a - // merged copy of the identifier on name. This will make sure that both - // identifier and name configs are respected, with identifier getting - // priority - struct output_config *temp = new_output_config(id_on_name); - merge_output_config(temp, oc_name); - merge_output_config(temp, oc_id); - list_add(config->output_configs, temp); - free(result->name); - result->name = strdup(id_on_name); - merge_output_config(result, temp); + char id[128]; + output_get_identifier(id, sizeof(id), sway_output); - sway_log(SWAY_DEBUG, "Generated output config \"%s\" (enabled: %d)" - " (%dx%d@%fHz position %d,%d scale %f transform %d) (bg %s %s)" - " (power %d) (max render time: %d)", result->name, result->enabled, - result->width, result->height, result->refresh_rate, - result->x, result->y, result->scale, result->transform, - result->background, result->background_option, result->power, - result->max_render_time); - } else if (oc_name) { - // No identifier config, just return a copy of the name config - free(result->name); - result->name = strdup(name); - merge_output_config(result, oc_name); - } else if (oc_id) { - // No name config, just return a copy of the identifier config - free(result->name); - result->name = strdup(identifier); - merge_output_config(result, oc_id); - } else { - i = list_seq_find(config->output_configs, output_name_cmp, "*"); - if (i >= 0) { - // No name or identifier config, but there is a wildcard config - free(result->name); - result->name = strdup("*"); - merge_output_config(result, config->output_configs->items[i]); - } else if (!config->reloading) { - // No name, identifier, or wildcard config. Since we are not - // reloading with defaults, the output config will be empty, so - // just return NULL - free_output_config(result); - result = NULL; - } + int i; + bool match = false; + if ((i = list_seq_find(config->output_configs, output_name_cmp, "*")) >= 0) { + match = true; + oc = config->output_configs->items[i]; + merge_output_config(result, oc); + } + if ((i = list_seq_find(config->output_configs, output_name_cmp, name)) >= 0) { + match = true; + oc = config->output_configs->items[i]; + merge_output_config(result, oc); + } + if ((i = list_seq_find(config->output_configs, output_name_cmp, id)) >= 0) { + match = true; + oc = config->output_configs->items[i]; + merge_output_config(result, oc); } - free(id_on_name); - return result; -} + if (!match && !config->reloading) { + // No name, identifier, or wildcard config. Since we are not + // reloading with defaults, the output config will be empty, so + // just return NULL + free_output_config(result); + return NULL; + } -struct output_config *find_output_config(struct sway_output *output) { - char id[128]; - output_get_identifier(id, sizeof(id), output); - return get_output_config(id, output); + return result; } bool apply_output_configs(struct matched_output_config *configs, From f11c5d562e3507a5e8b21491d61a6e43e81e43ad Mon Sep 17 00:00:00 2001 From: Simon Ser Date: Fri, 12 Apr 2024 18:42:50 +0200 Subject: [PATCH 2/3] config/output: fix NULL derefs in store_output_config() ../sway/config/output.c:33:21: runtime error: member access within null pointer of type 'struct sway_output' AddressSanitizer:DEADLYSIGNAL ================================================================= ==7856==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000080 (pc 0x63da8558205c bp 0x7ffdc35881a0 sp 0x7ffdc3588160 T0) ==7856==The signal is caused by a READ memory access. ==7856==Hint: address points to the zero page. #0 0x63da8558205c in output_get_identifier ../sway/config/output.c:33 #1 0x63da855865c3 in store_output_config ../sway/config/output.c:220 #2 0x63da855d4066 in cmd_output ../sway/commands/output.c:106 #3 0x63da8547f2e3 in config_command ../sway/commands.c:425 #4 0x63da8548f3fc in read_config ../sway/config.c:822 #5 0x63da8548a224 in load_config ../sway/config.c:435 #6 0x63da8548b065 in load_main_config ../sway/config.c:507 #7 0x63da854bee8d in main ../sway/main.c:351 #8 0x77e2ea643ccf (/usr/lib/libc.so.6+0x25ccf) (BuildId: c0caa0b7709d3369ee575fcd7d7d0b0fc48733af) #9 0x77e2ea643d89 in __libc_start_main (/usr/lib/libc.so.6+0x25d89) (BuildId: c0caa0b7709d3369ee575fcd7d7d0b0fc48733af) #10 0x63da8547ad64 in _start (/home/simon/src/sway/build/sway/sway+0x372d64) (BuildId: 3fa2e8838c1c32713b40aec6b1e84bbe4db5bde8) Fixes: 1267e47de913 ("config/output: Refactor handling of tiered configs") --- sway/config/output.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/sway/config/output.c b/sway/config/output.c index e5ff240ae..aab3f0bd9 100644 --- a/sway/config/output.c +++ b/sway/config/output.c @@ -217,7 +217,10 @@ void store_output_config(struct output_config *oc) { } char id[128]; - output_get_identifier(id, sizeof(id), output); + if (output) { + output_get_identifier(id, sizeof(id), output); + } + for (int i = 0; i < config->output_configs->length; i++) { struct output_config *old = config->output_configs->items[i]; @@ -240,7 +243,7 @@ void store_output_config(struct output_config *oc) { // If the new config matches an output's name, and the old config // matches on that output's identifier, supersede it. - if (strcmp(old->name, id) == 0 && + if (output && strcmp(old->name, id) == 0 && strcmp(oc->name, output->wlr_output->name) == 0) { supersede_output_config(old, oc); } From 087226d997c15f4df30542778854999c632642a3 Mon Sep 17 00:00:00 2001 From: Simon Ser Date: Fri, 12 Apr 2024 18:44:07 +0200 Subject: [PATCH 3/3] config/output: drop fast path in store_output_config() If there is no output currently connected, we still want to merge to any existing config. It shouldn't matter to iterate over the list of outputs to do nothing anwyays. --- sway/config/output.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/sway/config/output.c b/sway/config/output.c index aab3f0bd9..54af5d8e3 100644 --- a/sway/config/output.c +++ b/sway/config/output.c @@ -211,10 +211,6 @@ void store_output_config(struct output_config *oc) { bool merged = false; bool wildcard = strcmp(oc->name, "*") == 0; struct sway_output *output = wildcard ? NULL : output_by_name_or_id(oc->name); - if (!output && !wildcard) { - // There is no config by this name, just add it in - goto done; - } char id[128]; if (output) { @@ -249,7 +245,6 @@ void store_output_config(struct output_config *oc) { } } -done: sway_log(SWAY_DEBUG, "Config stored for output %s (enabled: %d) (%dx%d@%fHz " "position %d,%d scale %f subpixel %s transform %d) (bg %s %s) (power %d) " "(max render time: %d)",