[LTP] [PATCH 1/3] lib/tst_kconfig: Rewrite the parser internals

[Cyril Hrubis]

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.

-- 
Cyril Hrubis
chrubis@suse.cz