From 1a8abe80dabd6b723897fc507808db30b126b3a4 Mon Sep 17 00:00:00 2001 From: Vito Caputo Date: Mon, 8 May 2023 00:11:09 -0700 Subject: til_settings: rework setting get/add for bare values The core thing here is rather than turning a bare value into a key as I was doing before - we just leave the bare value as a bare value and its setting must be located positionally via get_value_by_idx since there's no key. Existing callers that used to get_key() positionally now get_value_by_idx() positionally all the same, except it's the value instead of the key. This is mostly done for things like the module or fb name at the front of a settings instance. The impetus for this change is partially just cosmetic/ergonomics, but it's also rather strange for what's really a key-less value to be treated as a value-less key. It was also awkward to talk/reason about on the road to recursive settings where bare values would be supported as a standalone settings instance if properly escaped... This also adds unescaping of keys, and adds a dependency on the somewhat linux-specific open_memstream() which may need changing in the future (see comments). --- src/main.c | 6 ++-- src/sdl_fb.c | 2 +- src/setup.c | 21 ++--------- src/til.c | 2 +- src/til_settings.c | 103 +++++++++++++++++++++++++++++++++++------------------ src/til_settings.h | 6 ++-- 6 files changed, 80 insertions(+), 60 deletions(-) (limited to 'src') diff --git a/src/main.c b/src/main.c index efc9d12..5165d71 100644 --- a/src/main.c +++ b/src/main.c @@ -83,7 +83,7 @@ static int setup_video(const til_settings_t *settings, til_setting_t **res_setti til_setting_t *setting; const char *video; - video = til_settings_get_key(settings, 0, &setting); + video = til_settings_get_value_by_idx(settings, 0, &setting); if (!video || !setting->desc) { til_setting_desc_t *desc; const char *values[] = { @@ -377,8 +377,8 @@ int main(int argc, const char *argv[]) exit_if(r && print_setup_as_args(&setup, !rototiller.args.gogogo) < 0, "unable to print setup"); - exit_if(!(rototiller.module = til_lookup_module(til_settings_get_key(setup.module_settings, 0, NULL))), - "unable to lookup module from settings \"%s\"", til_settings_get_key(setup.module_settings, 0, NULL)); + exit_if(!(rototiller.module = til_lookup_module(til_settings_get_value_by_idx(setup.module_settings, 0, NULL))), + "unable to lookup module from settings \"%s\"", til_settings_get_value_by_idx(setup.module_settings, 0, NULL)); exit_if((r = til_fb_new(fb_ops, setup.video_setup, NUM_FB_PAGES, &rototiller.fb)) < 0, "unable to create fb: %s", strerror(-r)); diff --git a/src/sdl_fb.c b/src/sdl_fb.c index 469fcb1..9eabbaf 100644 --- a/src/sdl_fb.c +++ b/src/sdl_fb.c @@ -73,7 +73,7 @@ static int sdl_fb_setup(const til_settings_t *settings, til_setting_t **res_sett res_desc); if (r) return r; - } else if ((size = til_settings_get_value(settings, "size", res_setting)) && !(*res_setting)->desc) { + } else if ((size = til_settings_get_value_by_key(settings, "size", res_setting)) && !(*res_setting)->desc) { /* if fullscreen=on AND size=WxH is specified, we'll do a more legacy style SDL fullscreen * where it tries to change the video mode. But if size is unspecified, it'll be a desktop * style fullscreen where it just uses a fullscreen window in the existing video mode, and diff --git a/src/setup.c b/src/setup.c index f85ac10..7c13ea0 100644 --- a/src/setup.c +++ b/src/setup.c @@ -6,21 +6,6 @@ #include "til_setup.h" #include "til_util.h" -/* add key=value, but if key is NULL, use value as key. */ -static int add_value(til_settings_t *settings, const char *key, const char *value) -{ - assert(settings); - - if (!key) { - key = value; - value = NULL; - } - - assert(key); - - return til_settings_add_value(settings, key, value, NULL); -} - /* returns negative on error, otherwise number of additions made to settings */ int setup_interactively(til_settings_t *settings, int (*setup_func)(const til_settings_t *settings, til_setting_t **res_setting, const til_setting_desc_t **res_desc, til_setup_t **res_setup), int defaults, til_setup_t **res_setup, const til_setting_desc_t **res_failed_desc) @@ -115,7 +100,7 @@ int setup_interactively(til_settings_t *settings, int (*setup_func)(const til_se if (*buf == '\n') { /* accept preferred */ - add_value(settings, desc->key, desc->preferred); + til_settings_add_value(settings, desc->key, desc->preferred, NULL); } else { buf[strlen(buf) - 1] = '\0'; @@ -131,7 +116,7 @@ int setup_interactively(til_settings_t *settings, int (*setup_func)(const til_se for (found = i = 0; desc->values[i]; i++) { if (i == j) { - add_value(settings, desc->key, desc->values[i]); + til_settings_add_value(settings, desc->key, desc->values[i], NULL); found = 1; break; } @@ -146,7 +131,7 @@ int setup_interactively(til_settings_t *settings, int (*setup_func)(const til_se } else { /* use typed input as setting, TODO: apply regex */ - add_value(settings, desc->key, buf); + til_settings_add_value(settings, desc->key, buf, NULL); } } _next: diff --git a/src/til.c b/src/til.c index 9053c2d..6c40207 100644 --- a/src/til.c +++ b/src/til.c @@ -270,7 +270,7 @@ int til_module_setup(const til_settings_t *settings, til_setting_t **res_setting const til_module_t *module; const char *name; - name = til_settings_get_key(settings, 0, &setting); + name = til_settings_get_value_by_idx(settings, 0, &setting); if (!name) { const char *values[nelems(modules) + 1] = {}; const char *annotations[nelems(modules) + 1] = {}; diff --git a/src/til_settings.c b/src/til_settings.c index b4da4fd..84feed4 100644 --- a/src/til_settings.c +++ b/src/til_settings.c @@ -38,6 +38,7 @@ typedef struct til_settings_t { typedef enum til_settings_fsm_state_t { TIL_SETTINGS_FSM_STATE_KEY, + TIL_SETTINGS_FSM_STATE_KEY_ESCAPED, TIL_SETTINGS_FSM_STATE_EQUAL, TIL_SETTINGS_FSM_STATE_VALUE, TIL_SETTINGS_FSM_STATE_VALUE_ESCAPED, @@ -45,27 +46,39 @@ typedef enum til_settings_fsm_state_t { } til_settings_fsm_state_t; -static int add_value(til_settings_t *settings, const char *key, const char *value, const til_setting_desc_t *desc) +static til_setting_t * add_setting(til_settings_t *settings, const char *key, const char *value, const til_setting_desc_t *desc) { + til_setting_t **new_settings; + til_setting_t *s; + assert(settings); + s = calloc(1, sizeof(til_setting_t)); + if (!s) + return NULL; + + new_settings = realloc(settings->settings, (settings->num + 1) * sizeof(til_setting_t *)); + if (!new_settings) { + free(s); + return NULL; + } + + settings->settings = new_settings; + settings->settings[settings->num] = s; + settings->settings[settings->num]->key = key; + settings->settings[settings->num]->value = value; + settings->settings[settings->num]->desc = desc; settings->num++; - /* TODO errors */ - settings->settings = realloc(settings->settings, settings->num * sizeof(til_setting_t *)); - settings->settings[settings->num - 1] = calloc(1, sizeof(til_setting_t)); - settings->settings[settings->num - 1]->key = key; - settings->settings[settings->num - 1]->value = value; - settings->settings[settings->num - 1]->desc = desc; - return 0; + return s; } /* split settings_string into a data structure */ til_settings_t * til_settings_new(const char *label, const char *settings_string) { - til_settings_fsm_state_t state = TIL_SETTINGS_FSM_STATE_KEY; - const char *p, *token; + til_settings_fsm_state_t state = TIL_SETTINGS_FSM_STATE_COMMA; + const char *p; til_settings_t *settings; FILE *value_fp; char *value_buf; @@ -84,31 +97,44 @@ til_settings_t * til_settings_new(const char *label, const char *settings_string if (!settings_string) return settings; - for (token = p = settings_string; ;p++) { + for (p = settings_string;;p++) { switch (state) { case TIL_SETTINGS_FSM_STATE_COMMA: - token = p; + value_fp = open_memstream(&value_buf, &value_sz); /* TODO FIXME: open_memstream() isn't portable */ + if (!value_fp) + goto _err; + state = TIL_SETTINGS_FSM_STATE_KEY; - break; + /* fallthrough */ case TIL_SETTINGS_FSM_STATE_KEY: - if (*p == '=' || *p == ',' || *p == '\0') { - add_value(settings, strndup(token, p - token), NULL, NULL); + if (*p == '\\') + state = TIL_SETTINGS_FSM_STATE_KEY_ESCAPED; + else if (*p == '=' || *p == ',' || *p == '\0') { + fclose(value_fp); - if (*p == '=') + if (*p == '=') { /* key= */ + (void) add_setting(settings, value_buf, NULL, NULL); state = TIL_SETTINGS_FSM_STATE_EQUAL; - else if (*p == ',') + } else { /* bare value */ + (void) add_setting(settings, NULL, value_buf, NULL); state = TIL_SETTINGS_FSM_STATE_COMMA; - } + } + } else + fputc(*p, value_fp); + break; + + case TIL_SETTINGS_FSM_STATE_KEY_ESCAPED: + fputc(*p, value_fp); + state = TIL_SETTINGS_FSM_STATE_KEY; break; case TIL_SETTINGS_FSM_STATE_EQUAL: - value_fp = open_memstream(&value_buf, &value_sz); + value_fp = open_memstream(&value_buf, &value_sz); /* TODO FIXME: open_memstream() isn't portable */ if (!value_fp) goto _err; - token = p; state = TIL_SETTINGS_FSM_STATE_VALUE; /* fallthrough, necessary to not leave NULL values for empty "key=\0" settings */ @@ -125,6 +151,13 @@ til_settings_t * til_settings_new(const char *label, const char *settings_string break; case TIL_SETTINGS_FSM_STATE_VALUE_ESCAPED: + /* TODO: note currently we just pass-through literally whatever char was escaped, + * but in cases like \n it should really be turned into 0xa so we can actually have + * a setting contain a raw newline post-unescaping (imagine a marquee module supporting + * arbitray text to be drawn, newlines would be ok yeah? should it be responsible for + * unescaping "\n" itself or just look for '\n' (0xa) to insert linefeeds? I think + * the latter...) But until there's a real need for that, this can just stay as-is. + */ fputc(*p, value_fp); state = TIL_SETTINGS_FSM_STATE_VALUE; break; @@ -166,13 +199,16 @@ til_settings_t * til_settings_free(til_settings_t *settings) } -/* find key= in settings, return dup of value side or NULL if missing */ -const char * til_settings_get_value(const til_settings_t *settings, const char *key, til_setting_t **res_setting) +/* find key= in settings, return value NULL if missing, optionally store setting @res_setting if found */ +const char * til_settings_get_value_by_key(const til_settings_t *settings, const char *key, til_setting_t **res_setting) { assert(settings); assert(key); for (int i = 0; i < settings->num; i++) { + if (!settings->settings[i]->key) + continue; + if (!strcasecmp(key, settings->settings[i]->key)) { if (res_setting) *res_setting = settings->settings[i]; @@ -185,16 +221,16 @@ const char * til_settings_get_value(const til_settings_t *settings, const char * } -/* return positional key from settings */ -const char * til_settings_get_key(const til_settings_t *settings, unsigned pos, til_setting_t **res_setting) +/* return positional value from settings, NULL if missing, optionally store setting @res_setting if found */ +const char * til_settings_get_value_by_idx(const til_settings_t *settings, unsigned idx, til_setting_t **res_setting) { assert(settings); - if (pos < settings->num) { + if (idx < settings->num) { if (res_setting) - *res_setting = settings->settings[pos]; + *res_setting = settings->settings[idx]; - return settings->settings[pos]->key; + return settings->settings[idx]->value; } return NULL; @@ -216,7 +252,7 @@ int til_settings_get_and_describe_value(const til_settings_t *settings, const ti assert(desc); assert(res_value); - value = til_settings_get_value(settings, desc->key, &setting); + value = til_settings_get_value_by_key(settings, desc->key, &setting); if (!value || !setting->desc) { int r; @@ -243,17 +279,16 @@ int til_settings_get_and_describe_value(const til_settings_t *settings, const ti } -/* add key=value to the settings, - * or just key if value is NULL. +/* add key,value as a new setting to settings, + * NULL keys and/or values are passed through as-is * desc may be NULL, it's simply passed along as a passenger. */ -/* returns < 0 on error */ -int til_settings_add_value(til_settings_t *settings, const char *key, const char *value, const til_setting_desc_t *desc) +/* returns the added setting, or NULL on error (ENOMEM) */ +til_setting_t * til_settings_add_value(til_settings_t *settings, const char *key, const char *value, const til_setting_desc_t *desc) { assert(settings); - assert(key); - return add_value(settings, strdup(key), value ? strdup(value) : NULL, desc); + return add_setting(settings, key ? strdup(key) : NULL, value ? strdup(value) : NULL, desc); } diff --git a/src/til_settings.h b/src/til_settings.h index 8911e6c..c9da235 100644 --- a/src/til_settings.h +++ b/src/til_settings.h @@ -37,9 +37,9 @@ struct til_setting_t { til_settings_t * til_settings_new(const char *label, const char *settings); til_settings_t * til_settings_free(til_settings_t *settings); -const char * til_settings_get_value(const til_settings_t *settings, const char *key, til_setting_t **res_setting); -const char * til_settings_get_key(const til_settings_t *settings, unsigned pos, til_setting_t **res_setting); -int til_settings_add_value(til_settings_t *settings, const char *key, const char *value, const til_setting_desc_t *desc); +const char * til_settings_get_value_by_key(const til_settings_t *settings, const char *key, til_setting_t **res_setting); +const char * til_settings_get_value_by_idx(const til_settings_t *settings, unsigned idx, til_setting_t **res_setting); +til_setting_t * til_settings_add_value(til_settings_t *settings, const char *key, const char *value, const til_setting_desc_t *desc); void til_settings_reset_descs(til_settings_t *settings); int til_settings_get_and_describe_value(const til_settings_t *settings, const til_setting_desc_t *desc, const char **res_value, til_setting_t **res_setting, const til_setting_desc_t **res_desc); char * til_settings_as_arg(const til_settings_t *settings); -- cgit v1.2.1