From 3ef4f9fd1e6ae233adb0bbfd90573623ee1059e6 Mon Sep 17 00:00:00 2001 From: Vito Caputo Date: Tue, 4 Jul 2023 20:50:56 -0700 Subject: modules/*: return invalid setting on -EINVAL in setup In the interests of improving error handling of interactive setups, return the setting that was invalid when setup returns -EINVAL. For now this is only supported for non-finalized/non-baking setup calls, i.e. (!res_setup). In the future I want this also supported for finalizing when res_setup is set. A lot of the -EINVAL returns are actually during that stage (or need to be added) due to that being when the more sensitive parsing occurs going from strings to native numeric types and such. The main reason it's not already being done in this commit is it'll churn quite a bit to get there, since the setup funcs don't generally have the setting pointers onhand at that phase. It'll require changing the string-value-centric local variables to instead all be the til_setting_t* holding those values. And if setup funcs aren't going to be so value-centric, the settings API will likely change to not even return the values directly anymore and only return the full-blown til_settings_t as the main return pointer, perhaps rid of their res_setting use even. Anyway, while here I did some random little cleanups too. --- src/modules/checkers/checkers.c | 60 +++++++++-------------------------------- 1 file changed, 12 insertions(+), 48 deletions(-) (limited to 'src/modules/checkers') diff --git a/src/modules/checkers/checkers.c b/src/modules/checkers/checkers.c index 9ea89cf..5d76a88 100644 --- a/src/modules/checkers/checkers.c +++ b/src/modules/checkers/checkers.c @@ -448,7 +448,7 @@ static int checkers_setup(const til_settings_t *settings, til_setting_t **res_se { const char *size; const char *pattern; - const char *fill_module; + const char *fill_module, *fill_module_name; const til_settings_t *fill_module_settings; til_setting_t *fill_module_setting; const char *dynamics; @@ -572,11 +572,9 @@ static int checkers_setup(const til_settings_t *settings, til_setting_t **res_se assert((*res_setting)->value_as_nested_settings); fill_module_settings = (*res_setting)->value_as_nested_settings; - fill_module = til_settings_get_value_by_idx(fill_module_settings, 0, &fill_module_setting); - if (!fill_module) - return -EINVAL; + fill_module_name = til_settings_get_value_by_idx(fill_module_settings, 0, &fill_module_setting); - if (!fill_module_setting->desc) { + if (!fill_module_name || !fill_module_setting->desc) { r = til_setting_desc_new(fill_module_settings, &(til_setting_spec_t){ .name = "Filled cell module name", @@ -587,60 +585,26 @@ static int checkers_setup(const til_settings_t *settings, til_setting_t **res_se if (r < 0) return r; - *res_setting = fill_module_setting; + *res_setting = fill_module_name ? fill_module_setting : NULL; return 1; } - if (strcasecmp(fill_module, "none")) { - const til_module_t *mod = til_lookup_module(fill_module); + if (strcasecmp(fill_module_name, "none")) { + const til_module_t *mod = til_lookup_module(fill_module_name); + + if (!mod) { + *res_setting = fill_module_setting; - if (!mod) return -EINVAL; + } if (mod->setup) { r = mod->setup(fill_module_settings, res_setting, res_desc, NULL); if (r) return r; - - /* XXX: note no res_setup was provided, so while yes the fill_module_settings are - * fully populated according to the setup's return value, we don't yet have the baked - * setup. That occurs below while baking the checkers res_setup. - */ } } - /* TODO: here we would do nested setup for fill_module via (*res_setting)->settings until that returned 0. - * It seems like we would just leave res_setup NULL for nested setup at this phase, then in our if (res_setup) - * branch down below the nested setup would have to recur with res_setup supplied but pointing at a nested - * setup pointer hosted within this setup. */ - - /* XXX but this is a little more complicated than simply performing exhaustive setup of the fill_module here. - * it's kind of like how in scripting languages you have two forms of interpolation while initializing variables; - * there's the immediate interpolation at the time of intialization, and there's the delayed interpolation at - * time of variable use. i.e. foo="$a $b" vs. foo:="$a $b", the latter may expand $a and $b once and store the - * result in foo, whereas the former may just store the literal "$a $b" in foo and (re)perform the expansion of - * $a and $b every time $foo is evaluated. - * - * should every module have its own separate setting or snowflake syntax to control exhaustive setup up-front - * vs. just taking what settings have been provided and randomizing what's not provided at context create time? - * in the case of checkers::fill_module it's arguably less relevant (though the current frontends don't have a - * randomize settings feature, so if exhaustive setup of fill_module were forced up-front using the existing frontends - * would mean loss of fill_module randomized setup) But that might be O.K. for something like checkers::fill_module - * - * where it becomes more relevant is something like rtv::channels, where the single rtv instance will repeatedly - * create+destroy channel contexts using the supplied (or omitted) settings. We may want to have the settings - * exhaustively applied once when rtv was intantiated, giving the frontend setup opportunity to explicitly configure - * the channels exhaustively. Or we may want to just partially specify some settings for the channels, and for - * the settings non-specified use the defaults OR for those unsepcified have them be randomized. Then there's the - * question of should the randomizing recur for every channel switch or just occur once at time of rtv instantiation? - * - * it feels like the module settings syntax should probably control this behavior in a general way so the modules don't - * start inventing their own approaches. Perhaps the module name at the start of the settings string could simply have - * an optional leading modifier character from a reserved set module names can't start with. Someting like %modulename - * for randomizing unspecified settings, : for evaluating settings just once up-front then reusing them, @modulename for - * accepting defaults for anything unspecified, and !modulename to fail if any settings aren't specified. Maybe also - * something like * for making the current qualifiers apply recursively. - */ r = til_settings_get_and_describe_value(settings, &(til_setting_spec_t){ @@ -758,8 +722,8 @@ static int checkers_setup(const til_settings_t *settings, til_setting_t **res_se return -EINVAL; } - if (strcasecmp(fill_module, "none")) { - setup->fill_module = til_lookup_module(fill_module); + if (strcasecmp(fill_module_name, "none")) { + setup->fill_module = til_lookup_module(fill_module_name); if (!setup->fill_module) { til_setup_free(&setup->til_setup); return -EINVAL; -- cgit v1.2.3