| Message ID | 20201020100910.10828-2-chrubis@suse.cz |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for kconfig constraints | expand |
Hello, Cyril Hrubis <chrubis@suse.cz> writes: > This patch changes the parser internals so that the parser results are > not tied to the kconfigs array from the tst_test structure anymore. > > This is a first step to a parser that can parse more complex > expressions. > > Signed-off-by: Cyril Hrubis <chrubis@suse.cz> > --- > include/tst_kconfig.h | 34 ++++---- > lib/tst_kconfig.c | 184 +++++++++++++++++++----------------------- > 2 files changed, 103 insertions(+), 115 deletions(-) > > diff --git a/include/tst_kconfig.h b/include/tst_kconfig.h > index 2d2cfd782..1bb21fea8 100644 > --- a/include/tst_kconfig.h > +++ b/include/tst_kconfig.h > @@ -6,27 +6,27 @@ > #ifndef TST_KCONFIG_H__ > #define TST_KCONFIG_H__ > > -struct tst_kconfig_res { > - char match; > - char *value; > +struct tst_kconfig_var { > + char id[64]; > + unsigned int id_len; > + char choice; This should probably be an enum to give the compiler more info. Even if we use the current char values as the enum entries. > + char *val; > }; > > /** > - * Reads a kernel config and parses it for values defined in kconfigs array. > + * > + * Reads a kernel config, parses it and writes results into an array of > + * tst_kconfig_var structures. > * > * The path to the kernel config should be autodetected in most of the cases as > * the code looks for know locations. It can be explicitely set/overrided with > * the KCONFIG_PATH environment variable as well. > * > - * The kcofings array is expected to contain strings in a format "CONFIG_FOO" > - * or "CONFIG_FOO=bar". The result array has to be suitably sized to fit the > - * results. > - * > - * @param kconfigs array of config strings to look for > - * @param results array to store results to > - * @param cnt size of the arrays > + * The caller has to initialize the tst_kconfig_var structure. The id has to be > + * filled with config variable name such as 'CONFIG_FOO', the id_len should > + * hold the id string length and the choice and val has to be zeroed. > * > - * The match in the tst_kconfig_res structure is set as follows: > + * After a call to this function each choice be set as follows: > * > * 'm' - config option set to m > * 'y' - config option set to y > @@ -34,11 +34,13 @@ struct tst_kconfig_res { > * 'n' - config option is not set > * 0 - config option not found > * > - * In the case that match is set to 'v' the value points to a newly allocated > - * string that holds the value. > + * In the case that match is set to 'v' the val pointer points to a newly > + * allocated string that holds the value. > + * > + * @param vars An array of caller initalized tst_kconfig_var structures. > + * @param vars_len Length of the vars array. > */ > -void tst_kconfig_read(const char *const kconfigs[], > - struct tst_kconfig_res results[], size_t cnt); > +void tst_kconfig_read(struct tst_kconfig_var vars[], size_t vars_len); > > /** > * Checks if required kernel configuration options are set in the kernel > diff --git a/lib/tst_kconfig.c b/lib/tst_kconfig.c > index d49187b6f..f80925cc9 100644 > --- a/lib/tst_kconfig.c > +++ b/lib/tst_kconfig.c > @@ -84,15 +84,6 @@ static void close_kconfig(FILE *fp) > fclose(fp); > } > > -struct match { > - /* match len, string length up to \0 or = */ > - size_t len; > - /* if set part of conf string after = */ > - const char *val; > - /* if set the config option was matched already */ > - int match; > -}; > - > static int is_set(const char *str, const char *val) > { > size_t vlen = strlen(val); > @@ -114,96 +105,70 @@ static int is_set(const char *str, const char *val) > } > } > > -static inline int match(struct match *match, const char *conf, > - struct tst_kconfig_res *result, const char *line) > +static inline int kconfig_parse_line(const char *line, > + struct tst_kconfig_var *vars, > + unsigned int vars_len) > { > - if (match->match) > - return 0; > + unsigned int i; > > - const char *cfg = strstr(line, "CONFIG_"); > + for (i = 0; i < vars_len; i++) { > + if (!strncmp(vars[i].id, line, vars[i].id_len)) { > + const char *val = &line[vars[i].id_len]; > + It is valid to have 'CONFIG_VAR = y'. We should probably tokenize the lines first to remove whitespace issues and expose the parser to all possible variable name symbols and values instead of just the ones which appear in our current tests. > + switch (val[0]) { > + case '=': > + break; > + case ' ': > + if (is_set(val, "is not set")) { > + vars[i].choice = 'n'; > + return 1; > + } Typically such lines begin with a comment '#' and I don't see where that is handled. Possibly this will only match non standard configs? > + return 1; > + /* vars[i].id may be prefix to longer config var */ > + default: > + return 0; > + } > > - if (!cfg) > - return 0; > + if (is_set(val, "=y")) { > + vars[i].choice = 'y'; > + return 1; > + } > > - if (strncmp(cfg, conf, match->len)) > - return 0; > + if (is_set(val, "=m")) { > + vars[i].choice = 'm'; > + return 1; > + } > > - const char *val = &cfg[match->len]; > + vars[i].choice = 'v'; > + vars[i].val = strndup(val+1, strlen(val)-2); > > - switch (cfg[match->len]) { > - case '=': > - break; > - case ' ': > - if (is_set(val, "is not set")) { > - result->match = 'n'; > - goto match; > + return 1; > } > - /* fall through */ > - default: > - return 0; > - } > - > - if (is_set(val, "=y")) { > - result->match = 'y'; > - goto match; > } > > - if (is_set(val, "=m")) { > - result->match = 'm'; > - goto match; > - } > - > - result->match = 'v'; > - result->value = strndup(val+1, strlen(val)-2); > - > -match: > - match->match = 1; > - return 1; > + return 0; > } > > -void tst_kconfig_read(const char *const *kconfigs, > - struct tst_kconfig_res results[], size_t cnt) > +void tst_kconfig_read(struct tst_kconfig_var vars[], size_t vars_len) > { > - struct match matches[cnt]; > - FILE *fp; > - unsigned int i, j; > - char buf[1024]; > - > - for (i = 0; i < cnt; i++) { > - const char *val = strchr(kconfigs[i], '='); > - > - if (strncmp("CONFIG_", kconfigs[i], 7)) > - tst_brk(TBROK, "Invalid config string '%s'", kconfigs[i]); > + char line[128]; > + unsigned int vars_found = 0; > > - matches[i].match = 0; > - matches[i].len = strlen(kconfigs[i]); > - > - if (val) { > - matches[i].val = val + 1; > - matches[i].len -= strlen(val); > - } > - > - results[i].match = 0; > - results[i].value = NULL; > - } > - > - fp = open_kconfig(); > + FILE *fp = open_kconfig(); > if (!fp) > tst_brk(TBROK, "Cannot parse kernel .config"); > > - while (fgets(buf, sizeof(buf), fp)) { > - for (i = 0; i < cnt; i++) { > - if (match(&matches[i], kconfigs[i], &results[i], buf)) { > - for (j = 0; j < cnt; j++) { > - if (matches[j].match) > - break; > - } > + while (fgets(line, sizeof(line), fp)) { > + char *cfg = strstr(line, "CONFIG_"); > > - if (j == cnt) > - goto exit; > - } > - } > + if (!cfg) > + continue; This filtering seems unecessary and maybe will hide some corner cases because it reduces kconfig_parses_line's exposure. Also practically every line has 'CONFIG_' in it. > + > + if (kconfig_parse_line(line, vars, vars_len)) > + vars_found++; > > + if (vars_found == vars_len) > + goto exit; > } Generally, this approach seems like to result in spurious TCONFs. We need to properly parse the file and fail if some line can't be interpreted. > > exit: > @@ -219,42 +184,63 @@ static size_t array_len(const char *const kconfigs[]) > return i; > } > > -static int compare_res(struct tst_kconfig_res *res, const char *kconfig, > - char match, const char *val) > +static int compare_res(struct tst_kconfig_var *var, const char *kconfig, > + char choice, const char *val) > { > - if (res->match != match) { > - tst_res(TINFO, "Needs kernel %s, have %c", kconfig, res->match); > + if (var->choice != choice) { > + tst_res(TINFO, "Needs kernel %s, have %c", kconfig, var->choice); > return 1; > } > > - if (match != 'v') > + if (choice != 'v') > return 0; > > - if (strcmp(res->value, val)) { > - tst_res(TINFO, "Needs kernel %s, have %s", kconfig, res->value); > + if (strcmp(var->val, val)) { > + tst_res(TINFO, "Needs kernel %s, have %s", kconfig, var->val); > return 1; > } > > return 0; > } > > +static inline unsigned int get_len(const char* kconfig) > +{ > + char *sep = index(kconfig, '='); > + > + if (!sep) > + return strlen(kconfig); > + > + return sep - kconfig; > +} > + > void tst_kconfig_check(const char *const kconfigs[]) > { > - size_t cnt = array_len(kconfigs); > - struct tst_kconfig_res results[cnt]; > + size_t vars_cnt = array_len(kconfigs); > + struct tst_kconfig_var vars[vars_cnt]; > unsigned int i; > int abort_test = 0; > > - tst_kconfig_read(kconfigs, results, cnt); > + memset(vars, 0, sizeof(*vars) * vars_cnt); > + > + for (i = 0; i < vars_cnt; i++) { > + vars[i].id_len = get_len(kconfigs[i]); > + > + if (vars[i].id_len >= sizeof(vars[i].id)) > + tst_brk(TBROK, "kconfig var id too long!"); > + > + strncpy(vars[i].id, kconfigs[i], vars[i].id_len); > + } > + > + tst_kconfig_read(vars, vars_cnt); > > - for (i = 0; i < cnt; i++) { > - if (results[i].match == 0) { > + for (i = 0; i < vars_cnt; i++) { > + if (vars[i].choice == 0) { > tst_res(TINFO, "Missing kernel %s", kconfigs[i]); > abort_test = 1; > continue; > } > > - if (results[i].match == 'n') { > + if (vars[i].choice == 'n') { > tst_res(TINFO, "Kernel %s is not set", kconfigs[i]); > abort_test = 1; > continue; > @@ -263,21 +249,21 @@ void tst_kconfig_check(const char *const kconfigs[]) > const char *val = strchr(kconfigs[i], '='); > > if (val) { > - char match = 'v'; > + char choice = 'v'; > val++; > > if (!strcmp(val, "y")) > - match = 'y'; > + choice = 'y'; > > if (!strcmp(val, "m")) > - match = 'm'; > + choice = 'm'; > > - if (compare_res(&results[i], kconfigs[i], match, val)) > + if (compare_res(&vars[i], kconfigs[i], choice, val)) > abort_test = 1; > > } > > - free(results[i].value); > + free(vars[i].val); > } > > if (abort_test) > -- > 2.26.2 I suppose most of the problems here stem from the original code, but your patch may as well clear it up :-)
Hi! > > diff --git a/include/tst_kconfig.h b/include/tst_kconfig.h > > index 2d2cfd782..1bb21fea8 100644 > > --- a/include/tst_kconfig.h > > +++ b/include/tst_kconfig.h > > @@ -6,27 +6,27 @@ > > #ifndef TST_KCONFIG_H__ > > #define TST_KCONFIG_H__ > > > > -struct tst_kconfig_res { > > - char match; > > - char *value; > > +struct tst_kconfig_var { > > + char id[64]; > > + unsigned int id_len; > > + char choice; > > This should probably be an enum to give the compiler more info. Even if > we use the current char values as the enum entries. Actually I was thinking of getting rid of this entirely in a follow up patch and just keep the val, because after the last patch these values are not special anymore. > > + char *val; > > }; > > > > /** > > - * Reads a kernel config and parses it for values defined in kconfigs array. > > + * > > + * Reads a kernel config, parses it and writes results into an array of > > + * tst_kconfig_var structures. > > * > > * The path to the kernel config should be autodetected in most of the cases as > > * the code looks for know locations. It can be explicitely set/overrided with > > * the KCONFIG_PATH environment variable as well. > > * > > - * The kcofings array is expected to contain strings in a format "CONFIG_FOO" > > - * or "CONFIG_FOO=bar". The result array has to be suitably sized to fit the > > - * results. > > - * > > - * @param kconfigs array of config strings to look for > > - * @param results array to store results to > > - * @param cnt size of the arrays > > + * The caller has to initialize the tst_kconfig_var structure. The id has to be > > + * filled with config variable name such as 'CONFIG_FOO', the id_len should > > + * hold the id string length and the choice and val has to be zeroed. > > * > > - * The match in the tst_kconfig_res structure is set as follows: > > + * After a call to this function each choice be set as follows: > > * > > * 'm' - config option set to m > > * 'y' - config option set to y > > @@ -34,11 +34,13 @@ struct tst_kconfig_res { > > * 'n' - config option is not set > > * 0 - config option not found > > * > > - * In the case that match is set to 'v' the value points to a newly allocated > > - * string that holds the value. > > + * In the case that match is set to 'v' the val pointer points to a newly > > + * allocated string that holds the value. > > + * > > + * @param vars An array of caller initalized tst_kconfig_var structures. > > + * @param vars_len Length of the vars array. > > */ > > -void tst_kconfig_read(const char *const kconfigs[], > > - struct tst_kconfig_res results[], size_t cnt); > > +void tst_kconfig_read(struct tst_kconfig_var vars[], size_t vars_len); > > > > /** > > * Checks if required kernel configuration options are set in the kernel > > diff --git a/lib/tst_kconfig.c b/lib/tst_kconfig.c > > index d49187b6f..f80925cc9 100644 > > --- a/lib/tst_kconfig.c > > +++ b/lib/tst_kconfig.c > > @@ -84,15 +84,6 @@ static void close_kconfig(FILE *fp) > > fclose(fp); > > } > > > > -struct match { > > - /* match len, string length up to \0 or = */ > > - size_t len; > > - /* if set part of conf string after = */ > > - const char *val; > > - /* if set the config option was matched already */ > > - int match; > > -}; > > - > > static int is_set(const char *str, const char *val) > > { > > size_t vlen = strlen(val); > > @@ -114,96 +105,70 @@ static int is_set(const char *str, const char *val) > > } > > } > > > > -static inline int match(struct match *match, const char *conf, > > - struct tst_kconfig_res *result, const char *line) > > +static inline int kconfig_parse_line(const char *line, > > + struct tst_kconfig_var *vars, > > + unsigned int vars_len) > > { > > - if (match->match) > > - return 0; > > + unsigned int i; > > > > - const char *cfg = strstr(line, "CONFIG_"); > > + for (i = 0; i < vars_len; i++) { > > + if (!strncmp(vars[i].id, line, vars[i].id_len)) { > > + const char *val = &line[vars[i].id_len]; > > + > > It is valid to have 'CONFIG_VAR = y'. We should probably tokenize the > lines first to remove whitespace issues and expose the parser to all > possible variable name symbols and values instead of just the ones which > appear in our current tests. I guess that it's techincally possible to have a whitespaces there, but will not happen unless you hand-edit the config file before compilation, which I doubt will ever happen. > > + switch (val[0]) { > > + case '=': > > + break; > > + case ' ': > > + if (is_set(val, "is not set")) { > > + vars[i].choice = 'n'; > > + return 1; > > + } > > Typically such lines begin with a comment '#' and I don't see where that > is handled. Possibly this will only match non standard configs? It does work actually, since we use strstr() to get the "CONFIG_" prefix from each line of the configuration, but I guess this needs to be fixed anyways since we would detect "# CONFIG_FOO=y" as enabled config feature even if it's commented. Again this will not happen unless you hand-edit the file, but it's probably worth fixing in a follow up patch. > > + return 1; > > + /* vars[i].id may be prefix to longer config var */ > > + default: > > + return 0; > > + } > > > > - if (!cfg) > > - return 0; > > + if (is_set(val, "=y")) { > > + vars[i].choice = 'y'; > > + return 1; > > + } > > > > - if (strncmp(cfg, conf, match->len)) > > - return 0; > > + if (is_set(val, "=m")) { > > + vars[i].choice = 'm'; > > + return 1; > > + } > > > > - const char *val = &cfg[match->len]; > > + vars[i].choice = 'v'; > > + vars[i].val = strndup(val+1, strlen(val)-2); > > > > - switch (cfg[match->len]) { > > - case '=': > > - break; > > - case ' ': > > - if (is_set(val, "is not set")) { > > - result->match = 'n'; > > - goto match; > > + return 1; > > } > > - /* fall through */ > > - default: > > - return 0; > > - } > > - > > - if (is_set(val, "=y")) { > > - result->match = 'y'; > > - goto match; > > } > > > > - if (is_set(val, "=m")) { > > - result->match = 'm'; > > - goto match; > > - } > > - > > - result->match = 'v'; > > - result->value = strndup(val+1, strlen(val)-2); > > - > > -match: > > - match->match = 1; > > - return 1; > > + return 0; > > } > > > > -void tst_kconfig_read(const char *const *kconfigs, > > - struct tst_kconfig_res results[], size_t cnt) > > +void tst_kconfig_read(struct tst_kconfig_var vars[], size_t vars_len) > > { > > - struct match matches[cnt]; > > - FILE *fp; > > - unsigned int i, j; > > - char buf[1024]; > > - > > - for (i = 0; i < cnt; i++) { > > - const char *val = strchr(kconfigs[i], '='); > > - > > - if (strncmp("CONFIG_", kconfigs[i], 7)) > > - tst_brk(TBROK, "Invalid config string '%s'", kconfigs[i]); > > + char line[128]; > > + unsigned int vars_found = 0; > > > > - matches[i].match = 0; > > - matches[i].len = strlen(kconfigs[i]); > > - > > - if (val) { > > - matches[i].val = val + 1; > > - matches[i].len -= strlen(val); > > - } > > - > > - results[i].match = 0; > > - results[i].value = NULL; > > - } > > - > > - fp = open_kconfig(); > > + FILE *fp = open_kconfig(); > > if (!fp) > > tst_brk(TBROK, "Cannot parse kernel .config"); > > > > - while (fgets(buf, sizeof(buf), fp)) { > > - for (i = 0; i < cnt; i++) { > > - if (match(&matches[i], kconfigs[i], &results[i], buf)) { > > - for (j = 0; j < cnt; j++) { > > - if (matches[j].match) > > - break; > > - } > > + while (fgets(line, sizeof(line), fp)) { > > + char *cfg = strstr(line, "CONFIG_"); > > > > - if (j == cnt) > > - goto exit; > > - } > > - } > > + if (!cfg) > > + continue; > > This filtering seems unecessary and maybe will hide some corner cases > because it reduces kconfig_parses_line's exposure. Also practically > every line has 'CONFIG_' in it. Not really, there are empty lines and plenty of comments in the file generated by kernel infrastructure. > > + > > + if (kconfig_parse_line(line, vars, vars_len)) > > + vars_found++; > > > > + if (vars_found == vars_len) > > + goto exit; > > } > > Generally, this approach seems like to result in spurious TCONFs. We > need to properly parse the file and fail if some line can't be > interpreted. Well we do expect well formatted .config file from a start, if you hand edit it and put whitespaces into unexpected places more things may fail. > > exit: > > @@ -219,42 +184,63 @@ static size_t array_len(const char *const kconfigs[]) > > return i; > > } > > > > -static int compare_res(struct tst_kconfig_res *res, const char *kconfig, > > - char match, const char *val) > > +static int compare_res(struct tst_kconfig_var *var, const char *kconfig, > > + char choice, const char *val) > > { > > - if (res->match != match) { > > - tst_res(TINFO, "Needs kernel %s, have %c", kconfig, res->match); > > + if (var->choice != choice) { > > + tst_res(TINFO, "Needs kernel %s, have %c", kconfig, var->choice); > > return 1; > > } > > > > - if (match != 'v') > > + if (choice != 'v') > > return 0; > > > > - if (strcmp(res->value, val)) { > > - tst_res(TINFO, "Needs kernel %s, have %s", kconfig, res->value); > > + if (strcmp(var->val, val)) { > > + tst_res(TINFO, "Needs kernel %s, have %s", kconfig, var->val); > > return 1; > > } > > > > return 0; > > } > > > > +static inline unsigned int get_len(const char* kconfig) > > +{ > > + char *sep = index(kconfig, '='); > > + > > + if (!sep) > > + return strlen(kconfig); > > + > > + return sep - kconfig; > > +} > > + > > void tst_kconfig_check(const char *const kconfigs[]) > > { > > - size_t cnt = array_len(kconfigs); > > - struct tst_kconfig_res results[cnt]; > > + size_t vars_cnt = array_len(kconfigs); > > + struct tst_kconfig_var vars[vars_cnt]; > > unsigned int i; > > int abort_test = 0; > > > > - tst_kconfig_read(kconfigs, results, cnt); > > + memset(vars, 0, sizeof(*vars) * vars_cnt); > > + > > + for (i = 0; i < vars_cnt; i++) { > > + vars[i].id_len = get_len(kconfigs[i]); > > + > > + if (vars[i].id_len >= sizeof(vars[i].id)) > > + tst_brk(TBROK, "kconfig var id too long!"); > > + > > + strncpy(vars[i].id, kconfigs[i], vars[i].id_len); > > + } > > + > > + tst_kconfig_read(vars, vars_cnt); > > > > - for (i = 0; i < cnt; i++) { > > - if (results[i].match == 0) { > > + for (i = 0; i < vars_cnt; i++) { > > + if (vars[i].choice == 0) { > > tst_res(TINFO, "Missing kernel %s", kconfigs[i]); > > abort_test = 1; > > continue; > > } > > > > - if (results[i].match == 'n') { > > + if (vars[i].choice == 'n') { > > tst_res(TINFO, "Kernel %s is not set", kconfigs[i]); > > abort_test = 1; > > continue; > > @@ -263,21 +249,21 @@ void tst_kconfig_check(const char *const kconfigs[]) > > const char *val = strchr(kconfigs[i], '='); > > > > if (val) { > > - char match = 'v'; > > + char choice = 'v'; > > val++; > > > > if (!strcmp(val, "y")) > > - match = 'y'; > > + choice = 'y'; > > > > if (!strcmp(val, "m")) > > - match = 'm'; > > + choice = 'm'; > > > > - if (compare_res(&results[i], kconfigs[i], match, val)) > > + if (compare_res(&vars[i], kconfigs[i], choice, val)) > > abort_test = 1; > > > > } > > > > - free(results[i].value); > > + free(vars[i].val); > > } > > > > if (abort_test) > > -- > > 2.26.2 > > I suppose most of the problems here stem from the original code, but > your patch may as well clear it up :-) Actually the clear way how to fix this is in a separate patch so that logical changes are split into different patches.
Hello, Cyril Hrubis <chrubis@suse.cz> writes: >> > >> > -static inline int match(struct match *match, const char *conf, >> > - struct tst_kconfig_res *result, const char *line) >> > +static inline int kconfig_parse_line(const char *line, >> > + struct tst_kconfig_var *vars, >> > + unsigned int vars_len) >> > { >> > - if (match->match) >> > - return 0; >> > + unsigned int i; >> > >> > - const char *cfg = strstr(line, "CONFIG_"); >> > + for (i = 0; i < vars_len; i++) { >> > + if (!strncmp(vars[i].id, line, vars[i].id_len)) { >> > + const char *val = &line[vars[i].id_len]; >> > + >> >> It is valid to have 'CONFIG_VAR = y'. We should probably tokenize the >> lines first to remove whitespace issues and expose the parser to all >> possible variable name symbols and values instead of just the ones which >> appear in our current tests. > > I guess that it's techincally possible to have a whitespaces there, but > will not happen unless you hand-edit the config file before compilation, > which I doubt will ever happen. > It can also happen if someone has their own script to modify the config. At any rate, if you are confident that it will never happen then there should be no problem failing hard if it does. >> > + switch (val[0]) { >> > + case '=': >> > + break; >> > + case ' ': >> > + if (is_set(val, "is not set")) { >> > + vars[i].choice = 'n'; >> > + return 1; >> > + } >> >> Typically such lines begin with a comment '#' and I don't see where that >> is handled. Possibly this will only match non standard configs? > > It does work actually, since we use strstr() to get the "CONFIG_" prefix > from each line of the configuration, but I guess this needs to be fixed > anyways since we would detect "# CONFIG_FOO=y" as enabled config feature > even if it's commented. Again this will not happen unless you hand-edit > the file, but it's probably worth fixing in a follow up patch. We don't actually use the result of strstr anymore? > >> > + return 1; >> > + /* vars[i].id may be prefix to longer config var */ >> > + default: >> > + return 0; >> > + } >> > >> > - if (!cfg) >> > - return 0; >> > + if (is_set(val, "=y")) { >> > + vars[i].choice = 'y'; >> > + return 1; >> > + } >> > >> > - if (strncmp(cfg, conf, match->len)) >> > - return 0; >> > + if (is_set(val, "=m")) { >> > + vars[i].choice = 'm'; >> > + return 1; >> > + } >> > >> > - const char *val = &cfg[match->len]; >> > + vars[i].choice = 'v'; >> > + vars[i].val = strndup(val+1, strlen(val)-2); >> > >> > - switch (cfg[match->len]) { >> > - case '=': >> > - break; >> > - case ' ': >> > - if (is_set(val, "is not set")) { >> > - result->match = 'n'; >> > - goto match; >> > + return 1; >> > } >> > - /* fall through */ >> > - default: >> > - return 0; >> > - } >> > - >> > - if (is_set(val, "=y")) { >> > - result->match = 'y'; >> > - goto match; >> > } >> > >> > - if (is_set(val, "=m")) { >> > - result->match = 'm'; >> > - goto match; >> > - } >> > - >> > - result->match = 'v'; >> > - result->value = strndup(val+1, strlen(val)-2); >> > - >> > -match: >> > - match->match = 1; >> > - return 1; >> > + return 0; >> > } >> > >> > -void tst_kconfig_read(const char *const *kconfigs, >> > - struct tst_kconfig_res results[], size_t cnt) >> > +void tst_kconfig_read(struct tst_kconfig_var vars[], size_t vars_len) >> > { >> > - struct match matches[cnt]; >> > - FILE *fp; >> > - unsigned int i, j; >> > - char buf[1024]; >> > - >> > - for (i = 0; i < cnt; i++) { >> > - const char *val = strchr(kconfigs[i], '='); >> > - >> > - if (strncmp("CONFIG_", kconfigs[i], 7)) >> > - tst_brk(TBROK, "Invalid config string '%s'", kconfigs[i]); >> > + char line[128]; >> > + unsigned int vars_found = 0; >> > >> > - matches[i].match = 0; >> > - matches[i].len = strlen(kconfigs[i]); >> > - >> > - if (val) { >> > - matches[i].val = val + 1; >> > - matches[i].len -= strlen(val); >> > - } >> > - >> > - results[i].match = 0; >> > - results[i].value = NULL; >> > - } >> > - >> > - fp = open_kconfig(); >> > + FILE *fp = open_kconfig(); >> > if (!fp) >> > tst_brk(TBROK, "Cannot parse kernel .config"); >> > >> > - while (fgets(buf, sizeof(buf), fp)) { >> > - for (i = 0; i < cnt; i++) { >> > - if (match(&matches[i], kconfigs[i], &results[i], buf)) { >> > - for (j = 0; j < cnt; j++) { >> > - if (matches[j].match) >> > - break; >> > - } >> > + while (fgets(line, sizeof(line), fp)) { >> > + char *cfg = strstr(line, "CONFIG_"); >> > >> > - if (j == cnt) >> > - goto exit; >> > - } >> > - } >> > + if (!cfg) >> > + continue; >> >> This filtering seems unecessary and maybe will hide some corner cases >> because it reduces kconfig_parses_line's exposure. Also practically >> every line has 'CONFIG_' in it. > > Not really, there are empty lines and plenty of comments in the file > generated by kernel infrastructure. It seems about 80-90% of lines contain CONFIG_, however if you pass it to kconfig_parse_line then this makes more sense. Still I think with proper parsing this shouldn't be there. > >> > + >> > + if (kconfig_parse_line(line, vars, vars_len)) >> > + vars_found++; >> > >> > + if (vars_found == vars_len) >> > + goto exit; >> > } >> >> Generally, this approach seems like to result in spurious TCONFs. We >> need to properly parse the file and fail if some line can't be >> interpreted. > > Well we do expect well formatted .config file from a start, if you hand > edit it and put whitespaces into unexpected places more things may > fail. Kernel build system seems to have no problem with it. More generally though we should fail hard if there is something unexpected, not produce TCONF which people don't check. > >> > exit: >> > @@ -219,42 +184,63 @@ static size_t array_len(const char *const kconfigs[]) >> > return i; >> > } >> > >> > -static int compare_res(struct tst_kconfig_res *res, const char *kconfig, >> > - char match, const char *val) >> > +static int compare_res(struct tst_kconfig_var *var, const char *kconfig, >> > + char choice, const char *val) >> > { >> > - if (res->match != match) { >> > - tst_res(TINFO, "Needs kernel %s, have %c", kconfig, res->match); >> > + if (var->choice != choice) { >> > + tst_res(TINFO, "Needs kernel %s, have %c", kconfig, var->choice); >> > return 1; >> > } >> > >> > - if (match != 'v') >> > + if (choice != 'v') >> > return 0; >> > >> > - if (strcmp(res->value, val)) { >> > - tst_res(TINFO, "Needs kernel %s, have %s", kconfig, res->value); >> > + if (strcmp(var->val, val)) { >> > + tst_res(TINFO, "Needs kernel %s, have %s", kconfig, var->val); >> > return 1; >> > } >> > >> > return 0; >> > } >> > >> > +static inline unsigned int get_len(const char* kconfig) >> > +{ >> > + char *sep = index(kconfig, '='); >> > + >> > + if (!sep) >> > + return strlen(kconfig); >> > + >> > + return sep - kconfig; >> > +} >> > + >> > void tst_kconfig_check(const char *const kconfigs[]) >> > { >> > - size_t cnt = array_len(kconfigs); >> > - struct tst_kconfig_res results[cnt]; >> > + size_t vars_cnt = array_len(kconfigs); >> > + struct tst_kconfig_var vars[vars_cnt]; >> > unsigned int i; >> > int abort_test = 0; >> > >> > - tst_kconfig_read(kconfigs, results, cnt); >> > + memset(vars, 0, sizeof(*vars) * vars_cnt); >> > + >> > + for (i = 0; i < vars_cnt; i++) { >> > + vars[i].id_len = get_len(kconfigs[i]); >> > + >> > + if (vars[i].id_len >= sizeof(vars[i].id)) >> > + tst_brk(TBROK, "kconfig var id too long!"); >> > + >> > + strncpy(vars[i].id, kconfigs[i], vars[i].id_len); >> > + } >> > + >> > + tst_kconfig_read(vars, vars_cnt); >> > >> > - for (i = 0; i < cnt; i++) { >> > - if (results[i].match == 0) { >> > + for (i = 0; i < vars_cnt; i++) { >> > + if (vars[i].choice == 0) { >> > tst_res(TINFO, "Missing kernel %s", kconfigs[i]); >> > abort_test = 1; >> > continue; >> > } >> > >> > - if (results[i].match == 'n') { >> > + if (vars[i].choice == 'n') { >> > tst_res(TINFO, "Kernel %s is not set", kconfigs[i]); >> > abort_test = 1; >> > continue; >> > @@ -263,21 +249,21 @@ void tst_kconfig_check(const char *const kconfigs[]) >> > const char *val = strchr(kconfigs[i], '='); >> > >> > if (val) { >> > - char match = 'v'; >> > + char choice = 'v'; >> > val++; >> > >> > if (!strcmp(val, "y")) >> > - match = 'y'; >> > + choice = 'y'; >> > >> > if (!strcmp(val, "m")) >> > - match = 'm'; >> > + choice = 'm'; >> > >> > - if (compare_res(&results[i], kconfigs[i], match, val)) >> > + if (compare_res(&vars[i], kconfigs[i], choice, val)) >> > abort_test = 1; >> > >> > } >> > >> > - free(results[i].value); >> > + free(vars[i].val); >> > } >> > >> > if (abort_test) >> > -- >> > 2.26.2 >> >> I suppose most of the problems here stem from the original code, but >> your patch may as well clear it up :-) > > Actually the clear way how to fix this is in a separate patch so that > logical changes are split into different patches. I suppose that elements of the boolean parser can be used to parse the kconfig and it can be combined (in a later patch). It's kind of unecessary to parse a config file into RPN, but will work perfectly well so we can share some code here.
Hi! > >> lines first to remove whitespace issues and expose the parser to all > >> possible variable name symbols and values instead of just the ones which > >> appear in our current tests. > > > > I guess that it's techincally possible to have a whitespaces there, but > > will not happen unless you hand-edit the config file before compilation, > > which I doubt will ever happen. > > > > It can also happen if someone has their own script to modify the > config. At any rate, if you are confident that it will never happen then > there should be no problem failing hard if it does. It would be probably easier to eat the whitespace around the = if present. But still I would ignore anything that isn't correct variable assignment, since such config would fail kernel compilation anyways. > >> > + switch (val[0]) { > >> > + case '=': > >> > + break; > >> > + case ' ': > >> > + if (is_set(val, "is not set")) { > >> > + vars[i].choice = 'n'; > >> > + return 1; > >> > + } > >> > >> Typically such lines begin with a comment '#' and I don't see where that > >> is handled. Possibly this will only match non standard configs? > > > > It does work actually, since we use strstr() to get the "CONFIG_" prefix > > from each line of the configuration, but I guess this needs to be fixed > > anyways since we would detect "# CONFIG_FOO=y" as enabled config feature > > even if it's commented. Again this will not happen unless you hand-edit > > the file, but it's probably worth fixing in a follow up patch. > > We don't actually use the result of strstr anymore? Ah right, that's a bug, the cfg should be passed to the kconfig_parse_line() instead, at least that's how the previous version worked in order to differentiate between unset and unknown variables. > >> > + return 1; > >> > + /* vars[i].id may be prefix to longer config var */ > >> > + default: > >> > + return 0; > >> > + } > >> > > >> > - if (!cfg) > >> > - return 0; > >> > + if (is_set(val, "=y")) { > >> > + vars[i].choice = 'y'; > >> > + return 1; > >> > + } > >> > > >> > - if (strncmp(cfg, conf, match->len)) > >> > - return 0; > >> > + if (is_set(val, "=m")) { > >> > + vars[i].choice = 'm'; > >> > + return 1; > >> > + } > >> > > >> > - const char *val = &cfg[match->len]; > >> > + vars[i].choice = 'v'; > >> > + vars[i].val = strndup(val+1, strlen(val)-2); > >> > > >> > - switch (cfg[match->len]) { > >> > - case '=': > >> > - break; > >> > - case ' ': > >> > - if (is_set(val, "is not set")) { > >> > - result->match = 'n'; > >> > - goto match; > >> > + return 1; > >> > } > >> > - /* fall through */ > >> > - default: > >> > - return 0; > >> > - } > >> > - > >> > - if (is_set(val, "=y")) { > >> > - result->match = 'y'; > >> > - goto match; > >> > } > >> > > >> > - if (is_set(val, "=m")) { > >> > - result->match = 'm'; > >> > - goto match; > >> > - } > >> > - > >> > - result->match = 'v'; > >> > - result->value = strndup(val+1, strlen(val)-2); > >> > - > >> > -match: > >> > - match->match = 1; > >> > - return 1; > >> > + return 0; > >> > } > >> > > >> > -void tst_kconfig_read(const char *const *kconfigs, > >> > - struct tst_kconfig_res results[], size_t cnt) > >> > +void tst_kconfig_read(struct tst_kconfig_var vars[], size_t vars_len) > >> > { > >> > - struct match matches[cnt]; > >> > - FILE *fp; > >> > - unsigned int i, j; > >> > - char buf[1024]; > >> > - > >> > - for (i = 0; i < cnt; i++) { > >> > - const char *val = strchr(kconfigs[i], '='); > >> > - > >> > - if (strncmp("CONFIG_", kconfigs[i], 7)) > >> > - tst_brk(TBROK, "Invalid config string '%s'", kconfigs[i]); > >> > + char line[128]; > >> > + unsigned int vars_found = 0; > >> > > >> > - matches[i].match = 0; > >> > - matches[i].len = strlen(kconfigs[i]); > >> > - > >> > - if (val) { > >> > - matches[i].val = val + 1; > >> > - matches[i].len -= strlen(val); > >> > - } > >> > - > >> > - results[i].match = 0; > >> > - results[i].value = NULL; > >> > - } > >> > - > >> > - fp = open_kconfig(); > >> > + FILE *fp = open_kconfig(); > >> > if (!fp) > >> > tst_brk(TBROK, "Cannot parse kernel .config"); > >> > > >> > - while (fgets(buf, sizeof(buf), fp)) { > >> > - for (i = 0; i < cnt; i++) { > >> > - if (match(&matches[i], kconfigs[i], &results[i], buf)) { > >> > - for (j = 0; j < cnt; j++) { > >> > - if (matches[j].match) > >> > - break; > >> > - } > >> > + while (fgets(line, sizeof(line), fp)) { > >> > + char *cfg = strstr(line, "CONFIG_"); > >> > > >> > - if (j == cnt) > >> > - goto exit; > >> > - } > >> > - } > >> > + if (!cfg) > >> > + continue; > >> > >> This filtering seems unecessary and maybe will hide some corner cases > >> because it reduces kconfig_parses_line's exposure. Also practically > >> every line has 'CONFIG_' in it. > > > > Not really, there are empty lines and plenty of comments in the file > > generated by kernel infrastructure. > > It seems about 80-90% of lines contain CONFIG_, however if you pass it > to kconfig_parse_line then this makes more sense. Still I think with > proper parsing this shouldn't be there. What exactly do you mean by a proper parsing? The file is format is very simple each line starts either with # which is a comment or consists of 'key=val' pair and the key is by definition prefixed with CONFIG_. > >> > + > >> > + if (kconfig_parse_line(line, vars, vars_len)) > >> > + vars_found++; > >> > > >> > + if (vars_found == vars_len) > >> > + goto exit; > >> > } > >> > >> Generally, this approach seems like to result in spurious TCONFs. We > >> need to properly parse the file and fail if some line can't be > >> interpreted. > > > > Well we do expect well formatted .config file from a start, if you hand > > edit it and put whitespaces into unexpected places more things may > > fail. > > Kernel build system seems to have no problem with it. More generally > though we should fail hard if there is something unexpected, not produce > TCONF which people don't check. Even if we do I do not think that we should care about anything but syntactically correct input, since if you modify the file after kernel compilation you have lost anyways. > >> I suppose most of the problems here stem from the original code, but > >> your patch may as well clear it up :-) > > > > Actually the clear way how to fix this is in a separate patch so that > > logical changes are split into different patches. > > I suppose that elements of the boolean parser can be used to parse the > kconfig and it can be combined (in a later patch). It's kind of > unecessary to parse a config file into RPN, but will work perfectly well > so we can share some code here. I do not get what you are trying to say. Are you saying that we should tokenize the input? I do not think that this is necessary since the file format is pretty simple.
Hi, > > >> lines first to remove whitespace issues and expose the parser to all > > >> possible variable name symbols and values instead of just the ones which > > >> appear in our current tests. > > > I guess that it's techincally possible to have a whitespaces there, but > > > will not happen unless you hand-edit the config file before compilation, > > > which I doubt will ever happen. > > It can also happen if someone has their own script to modify the > > config. At any rate, if you are confident that it will never happen then > > there should be no problem failing hard if it does. > It would be probably easier to eat the whitespace around the = if > present. But still I would ignore anything that isn't correct variable > assignment, since such config would fail kernel compilation anyways. +1 + I use ./scripts/config --enable | --disable ... to avoid problems with incorrect config. > > >> > + > > >> > + if (kconfig_parse_line(line, vars, vars_len)) > > >> > + vars_found++; > > >> > + if (vars_found == vars_len) > > >> > + goto exit; > > >> > } > > >> Generally, this approach seems like to result in spurious TCONFs. We > > >> need to properly parse the file and fail if some line can't be > > >> interpreted. > > > Well we do expect well formatted .config file from a start, if you hand > > > edit it and put whitespaces into unexpected places more things may > > > fail. > > Kernel build system seems to have no problem with it. More generally > > though we should fail hard if there is something unexpected, not produce > > TCONF which people don't check. > Even if we do I do not think that we should care about anything but > syntactically correct input, since if you modify the file after kernel > compilation you have lost anyways. +1 Kind regards, Petr
Hello, Cyril Hrubis <chrubis@suse.cz> writes: > Hi! >> >> lines first to remove whitespace issues and expose the parser to all >> >> possible variable name symbols and values instead of just the ones which >> >> appear in our current tests. >> > >> > I guess that it's techincally possible to have a whitespaces there, but >> > will not happen unless you hand-edit the config file before compilation, >> > which I doubt will ever happen. >> > >> >> It can also happen if someone has their own script to modify the >> config. At any rate, if you are confident that it will never happen then >> there should be no problem failing hard if it does. > > It would be probably easier to eat the whitespace around the = if > present. But still I would ignore anything that isn't correct variable > assignment, since such config would fail kernel compilation anyways. Works for me :-) > >> >> > + switch (val[0]) { >> >> > + case '=': >> >> > + break; >> >> > + case ' ': >> >> > + if (is_set(val, "is not set")) { >> >> > + vars[i].choice = 'n'; >> >> > + return 1; >> >> > + } >> >> >> >> Typically such lines begin with a comment '#' and I don't see where that >> >> is handled. Possibly this will only match non standard configs? >> > >> > It does work actually, since we use strstr() to get the "CONFIG_" prefix >> > from each line of the configuration, but I guess this needs to be fixed >> > anyways since we would detect "# CONFIG_FOO=y" as enabled config feature >> > even if it's commented. Again this will not happen unless you hand-edit >> > the file, but it's probably worth fixing in a follow up patch. >> >> We don't actually use the result of strstr anymore? > > Ah right, that's a bug, the cfg should be passed to the > kconfig_parse_line() instead, at least that's how the previous version > worked in order to differentiate between unset and unknown variables. > >> >> > + return 1; >> >> > + /* vars[i].id may be prefix to longer config var */ >> >> > + default: >> >> > + return 0; >> >> > + } >> >> > >> >> > - if (!cfg) >> >> > - return 0; >> >> > + if (is_set(val, "=y")) { >> >> > + vars[i].choice = 'y'; >> >> > + return 1; >> >> > + } >> >> > >> >> > - if (strncmp(cfg, conf, match->len)) >> >> > - return 0; >> >> > + if (is_set(val, "=m")) { >> >> > + vars[i].choice = 'm'; >> >> > + return 1; >> >> > + } >> >> > >> >> > - const char *val = &cfg[match->len]; >> >> > + vars[i].choice = 'v'; >> >> > + vars[i].val = strndup(val+1, strlen(val)-2); >> >> > >> >> > - switch (cfg[match->len]) { >> >> > - case '=': >> >> > - break; >> >> > - case ' ': >> >> > - if (is_set(val, "is not set")) { >> >> > - result->match = 'n'; >> >> > - goto match; >> >> > + return 1; >> >> > } >> >> > - /* fall through */ >> >> > - default: >> >> > - return 0; >> >> > - } >> >> > - >> >> > - if (is_set(val, "=y")) { >> >> > - result->match = 'y'; >> >> > - goto match; >> >> > } >> >> > >> >> > - if (is_set(val, "=m")) { >> >> > - result->match = 'm'; >> >> > - goto match; >> >> > - } >> >> > - >> >> > - result->match = 'v'; >> >> > - result->value = strndup(val+1, strlen(val)-2); >> >> > - >> >> > -match: >> >> > - match->match = 1; >> >> > - return 1; >> >> > + return 0; >> >> > } >> >> > >> >> > -void tst_kconfig_read(const char *const *kconfigs, >> >> > - struct tst_kconfig_res results[], size_t cnt) >> >> > +void tst_kconfig_read(struct tst_kconfig_var vars[], size_t vars_len) >> >> > { >> >> > - struct match matches[cnt]; >> >> > - FILE *fp; >> >> > - unsigned int i, j; >> >> > - char buf[1024]; >> >> > - >> >> > - for (i = 0; i < cnt; i++) { >> >> > - const char *val = strchr(kconfigs[i], '='); >> >> > - >> >> > - if (strncmp("CONFIG_", kconfigs[i], 7)) >> >> > - tst_brk(TBROK, "Invalid config string '%s'", kconfigs[i]); >> >> > + char line[128]; >> >> > + unsigned int vars_found = 0; >> >> > >> >> > - matches[i].match = 0; >> >> > - matches[i].len = strlen(kconfigs[i]); >> >> > - >> >> > - if (val) { >> >> > - matches[i].val = val + 1; >> >> > - matches[i].len -= strlen(val); >> >> > - } >> >> > - >> >> > - results[i].match = 0; >> >> > - results[i].value = NULL; >> >> > - } >> >> > - >> >> > - fp = open_kconfig(); >> >> > + FILE *fp = open_kconfig(); >> >> > if (!fp) >> >> > tst_brk(TBROK, "Cannot parse kernel .config"); >> >> > >> >> > - while (fgets(buf, sizeof(buf), fp)) { >> >> > - for (i = 0; i < cnt; i++) { >> >> > - if (match(&matches[i], kconfigs[i], &results[i], buf)) { >> >> > - for (j = 0; j < cnt; j++) { >> >> > - if (matches[j].match) >> >> > - break; >> >> > - } >> >> > + while (fgets(line, sizeof(line), fp)) { >> >> > + char *cfg = strstr(line, "CONFIG_"); >> >> > >> >> > - if (j == cnt) >> >> > - goto exit; >> >> > - } >> >> > - } >> >> > + if (!cfg) >> >> > + continue; >> >> >> >> This filtering seems unecessary and maybe will hide some corner cases >> >> because it reduces kconfig_parses_line's exposure. Also practically >> >> every line has 'CONFIG_' in it. >> > >> > Not really, there are empty lines and plenty of comments in the file >> > generated by kernel infrastructure. >> >> It seems about 80-90% of lines contain CONFIG_, however if you pass it >> to kconfig_parse_line then this makes more sense. Still I think with >> proper parsing this shouldn't be there. > > What exactly do you mean by a proper parsing? Tokenize it and remove whitespace. > > The file is format is very simple each line starts either with # which > is a comment or consists of 'key=val' pair and the key is by definition > prefixed with CONFIG_. > >> >> > + >> >> > + if (kconfig_parse_line(line, vars, vars_len)) >> >> > + vars_found++; >> >> > >> >> > + if (vars_found == vars_len) >> >> > + goto exit; >> >> > } >> >> >> >> Generally, this approach seems like to result in spurious TCONFs. We >> >> need to properly parse the file and fail if some line can't be >> >> interpreted. >> > >> > Well we do expect well formatted .config file from a start, if you hand >> > edit it and put whitespaces into unexpected places more things may >> > fail. >> >> Kernel build system seems to have no problem with it. More generally >> though we should fail hard if there is something unexpected, not produce >> TCONF which people don't check. > > Even if we do I do not think that we should care about anything but > syntactically correct input, since if you modify the file after kernel > compilation you have lost anyways. > >> >> I suppose most of the problems here stem from the original code, but >> >> your patch may as well clear it up :-) >> > >> > Actually the clear way how to fix this is in a separate patch so that >> > logical changes are split into different patches. >> >> I suppose that elements of the boolean parser can be used to parse the >> kconfig and it can be combined (in a later patch). It's kind of >> unecessary to parse a config file into RPN, but will work perfectly well >> so we can share some code here. > > I do not get what you are trying to say. Are you saying that we should > tokenize the input? I do not think that this is necessary since the file > format is pretty simple. Yes or atleast test the assumption you won't find configs with spaces or other wierd stuff in by throwing an error if we find something unexpected. If someone has a broken config we can tell them that.
Hi! > > I do not get what you are trying to say. Are you saying that we should > > tokenize the input? I do not think that this is necessary since the file > > format is pretty simple. > > Yes or atleast test the assumption you won't find configs with spaces or > other wierd stuff in by throwing an error if we find something > unexpected. If someone has a broken config we can tell them that. I guess I can make it a bit more robust in a follow up patch.
diff --git a/include/tst_kconfig.h b/include/tst_kconfig.h index 2d2cfd782..1bb21fea8 100644 --- a/include/tst_kconfig.h +++ b/include/tst_kconfig.h @@ -6,27 +6,27 @@ #ifndef TST_KCONFIG_H__ #define TST_KCONFIG_H__ -struct tst_kconfig_res { - char match; - char *value; +struct tst_kconfig_var { + char id[64]; + unsigned int id_len; + char choice; + char *val; }; /** - * Reads a kernel config and parses it for values defined in kconfigs array. + * + * Reads a kernel config, parses it and writes results into an array of + * tst_kconfig_var structures. * * The path to the kernel config should be autodetected in most of the cases as * the code looks for know locations. It can be explicitely set/overrided with * the KCONFIG_PATH environment variable as well. * - * The kcofings array is expected to contain strings in a format "CONFIG_FOO" - * or "CONFIG_FOO=bar". The result array has to be suitably sized to fit the - * results. - * - * @param kconfigs array of config strings to look for - * @param results array to store results to - * @param cnt size of the arrays + * The caller has to initialize the tst_kconfig_var structure. The id has to be + * filled with config variable name such as 'CONFIG_FOO', the id_len should + * hold the id string length and the choice and val has to be zeroed. * - * The match in the tst_kconfig_res structure is set as follows: + * After a call to this function each choice be set as follows: * * 'm' - config option set to m * 'y' - config option set to y @@ -34,11 +34,13 @@ struct tst_kconfig_res { * 'n' - config option is not set * 0 - config option not found * - * In the case that match is set to 'v' the value points to a newly allocated - * string that holds the value. + * In the case that match is set to 'v' the val pointer points to a newly + * allocated string that holds the value. + * + * @param vars An array of caller initalized tst_kconfig_var structures. + * @param vars_len Length of the vars array. */ -void tst_kconfig_read(const char *const kconfigs[], - struct tst_kconfig_res results[], size_t cnt); +void tst_kconfig_read(struct tst_kconfig_var vars[], size_t vars_len); /** * Checks if required kernel configuration options are set in the kernel diff --git a/lib/tst_kconfig.c b/lib/tst_kconfig.c index d49187b6f..f80925cc9 100644 --- a/lib/tst_kconfig.c +++ b/lib/tst_kconfig.c @@ -84,15 +84,6 @@ static void close_kconfig(FILE *fp) fclose(fp); } -struct match { - /* match len, string length up to \0 or = */ - size_t len; - /* if set part of conf string after = */ - const char *val; - /* if set the config option was matched already */ - int match; -}; - static int is_set(const char *str, const char *val) { size_t vlen = strlen(val); @@ -114,96 +105,70 @@ static int is_set(const char *str, const char *val) } } -static inline int match(struct match *match, const char *conf, - struct tst_kconfig_res *result, const char *line) +static inline int kconfig_parse_line(const char *line, + struct tst_kconfig_var *vars, + unsigned int vars_len) { - if (match->match) - return 0; + unsigned int i; - const char *cfg = strstr(line, "CONFIG_"); + for (i = 0; i < vars_len; i++) { + if (!strncmp(vars[i].id, line, vars[i].id_len)) { + const char *val = &line[vars[i].id_len]; + + switch (val[0]) { + case '=': + break; + case ' ': + if (is_set(val, "is not set")) { + vars[i].choice = 'n'; + return 1; + } + return 1; + /* vars[i].id may be prefix to longer config var */ + default: + return 0; + } - if (!cfg) - return 0; + if (is_set(val, "=y")) { + vars[i].choice = 'y'; + return 1; + } - if (strncmp(cfg, conf, match->len)) - return 0; + if (is_set(val, "=m")) { + vars[i].choice = 'm'; + return 1; + } - const char *val = &cfg[match->len]; + vars[i].choice = 'v'; + vars[i].val = strndup(val+1, strlen(val)-2); - switch (cfg[match->len]) { - case '=': - break; - case ' ': - if (is_set(val, "is not set")) { - result->match = 'n'; - goto match; + return 1; } - /* fall through */ - default: - return 0; - } - - if (is_set(val, "=y")) { - result->match = 'y'; - goto match; } - if (is_set(val, "=m")) { - result->match = 'm'; - goto match; - } - - result->match = 'v'; - result->value = strndup(val+1, strlen(val)-2); - -match: - match->match = 1; - return 1; + return 0; } -void tst_kconfig_read(const char *const *kconfigs, - struct tst_kconfig_res results[], size_t cnt) +void tst_kconfig_read(struct tst_kconfig_var vars[], size_t vars_len) { - struct match matches[cnt]; - FILE *fp; - unsigned int i, j; - char buf[1024]; - - for (i = 0; i < cnt; i++) { - const char *val = strchr(kconfigs[i], '='); - - if (strncmp("CONFIG_", kconfigs[i], 7)) - tst_brk(TBROK, "Invalid config string '%s'", kconfigs[i]); + char line[128]; + unsigned int vars_found = 0; - matches[i].match = 0; - matches[i].len = strlen(kconfigs[i]); - - if (val) { - matches[i].val = val + 1; - matches[i].len -= strlen(val); - } - - results[i].match = 0; - results[i].value = NULL; - } - - fp = open_kconfig(); + FILE *fp = open_kconfig(); if (!fp) tst_brk(TBROK, "Cannot parse kernel .config"); - while (fgets(buf, sizeof(buf), fp)) { - for (i = 0; i < cnt; i++) { - if (match(&matches[i], kconfigs[i], &results[i], buf)) { - for (j = 0; j < cnt; j++) { - if (matches[j].match) - break; - } + while (fgets(line, sizeof(line), fp)) { + char *cfg = strstr(line, "CONFIG_"); - if (j == cnt) - goto exit; - } - } + if (!cfg) + continue; + + if (kconfig_parse_line(line, vars, vars_len)) + vars_found++; + if (vars_found == vars_len) + goto exit; } exit: @@ -219,42 +184,63 @@ static size_t array_len(const char *const kconfigs[]) return i; } -static int compare_res(struct tst_kconfig_res *res, const char *kconfig, - char match, const char *val) +static int compare_res(struct tst_kconfig_var *var, const char *kconfig, + char choice, const char *val) { - if (res->match != match) { - tst_res(TINFO, "Needs kernel %s, have %c", kconfig, res->match); + if (var->choice != choice) { + tst_res(TINFO, "Needs kernel %s, have %c", kconfig, var->choice); return 1; } - if (match != 'v') + if (choice != 'v') return 0; - if (strcmp(res->value, val)) { - tst_res(TINFO, "Needs kernel %s, have %s", kconfig, res->value); + if (strcmp(var->val, val)) { + tst_res(TINFO, "Needs kernel %s, have %s", kconfig, var->val); return 1; } return 0; } +static inline unsigned int get_len(const char* kconfig) +{ + char *sep = index(kconfig, '='); + + if (!sep) + return strlen(kconfig); + + return sep - kconfig; +} + void tst_kconfig_check(const char *const kconfigs[]) { - size_t cnt = array_len(kconfigs); - struct tst_kconfig_res results[cnt]; + size_t vars_cnt = array_len(kconfigs); + struct tst_kconfig_var vars[vars_cnt]; unsigned int i; int abort_test = 0; - tst_kconfig_read(kconfigs, results, cnt); + memset(vars, 0, sizeof(*vars) * vars_cnt); + + for (i = 0; i < vars_cnt; i++) { + vars[i].id_len = get_len(kconfigs[i]); + + if (vars[i].id_len >= sizeof(vars[i].id)) + tst_brk(TBROK, "kconfig var id too long!"); + + strncpy(vars[i].id, kconfigs[i], vars[i].id_len); + } + + tst_kconfig_read(vars, vars_cnt); - for (i = 0; i < cnt; i++) { - if (results[i].match == 0) { + for (i = 0; i < vars_cnt; i++) { + if (vars[i].choice == 0) { tst_res(TINFO, "Missing kernel %s", kconfigs[i]); abort_test = 1; continue; } - if (results[i].match == 'n') { + if (vars[i].choice == 'n') { tst_res(TINFO, "Kernel %s is not set", kconfigs[i]); abort_test = 1; continue; @@ -263,21 +249,21 @@ void tst_kconfig_check(const char *const kconfigs[]) const char *val = strchr(kconfigs[i], '='); if (val) { - char match = 'v'; + char choice = 'v'; val++; if (!strcmp(val, "y")) - match = 'y'; + choice = 'y'; if (!strcmp(val, "m")) - match = 'm'; + choice = 'm'; - if (compare_res(&results[i], kconfigs[i], match, val)) + if (compare_res(&vars[i], kconfigs[i], choice, val)) abort_test = 1; } - free(results[i].value); + free(vars[i].val); } if (abort_test)
This patch changes the parser internals so that the parser results are not tied to the kconfigs array from the tst_test structure anymore. This is a first step to a parser that can parse more complex expressions. Signed-off-by: Cyril Hrubis <chrubis@suse.cz> --- include/tst_kconfig.h | 34 ++++---- lib/tst_kconfig.c | 184 +++++++++++++++++++----------------------- 2 files changed, 103 insertions(+), 115 deletions(-)