[LTP] [PATCH 1/3] lib/tst_kconfig: Rewrite the parser internals
Cyril Hrubis
chrubis@suse.cz
Wed Oct 21 12:06:05 CEST 2020
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.
--
Cyril Hrubis
chrubis@suse.cz
More information about the ltp
mailing list