Re: [LTP] [PATCH v1] float: convert to new LTP API

[Petr Vorel <pvorel@suse.cz>]

Hi Hongchen,

thank you for your effort, but much more needs to be done.

Code in whole project (all sources in testcases/misc/math/float) is very old and
IMHO weird. I still wonder why testing float / math functions requires creating
files, using pthread (in thread_code.c), ... It'd be worth to have look whether
current approach is really useful before spending time to rewrite it.

Some notes to your rewrite. main.c and thread_code.c should be turned into
header file (e.g. float.h) with functions being static inline. Because
including C files is no-go.

>  testcases/misc/math/float/main.c | 446 +++++++++------------------------------
...
> -const int nb_func = NB_FUNC;
> +static char *Dopt, *lopt, *nopt, *vopt;
> +static struct tst_option opt[] = {
> +	{"D:", &Dopt,   "DATA directory's absolute path"},
IMHO this should not be needed, everything should be in test temporary
directory, there is no need to put it elsewhere.
> +	{"l:", &lopt, "set number of loops per function"},
> +	{"n:", &nopt, "set number of threads per function"},
> +	{"v", &vopt, "debug level"},
I'd get rid of the debugging (important things should be always printed).

> +	{}
> +};

>  int generate(char *datadir, char *bin_path)
>  {
>  	char *cmdline;
>  	char *fmt = "cd %s; %s/%s %s";

> -	cmdline = malloc(2 * strlen(bin_path) + strlen(datadir) + strlen(GENERATOR) + strlen(fmt));
> +	cmdline = malloc(2 * strlen(bin_path) + strlen(datadir) +
> +				strlen(GENERATOR) + strlen(fmt));
>  	if (cmdline == NULL)
>  		return (1);
There is SAFE_MALLOC(), no need to check for NULL.

>  	sprintf(cmdline, fmt, datadir, bin_path, GENERATOR, bin_path);
> @@ -93,345 +55,137 @@ int generate(char *datadir, char *bin_path)
>  	return (0);

Also code style suggests it's very old. brackets around integer in return is
quite strange (i.e. "return (0);").
>  }


>  	ltproot = getenv("LTPROOT");
>  	if (ltproot == NULL || strlen(ltproot) == 0) {
> -		tst_brkm(TBROK, NULL,
> +		tst_brk(TBROK,
>  			 "You must set $LTPROOT before executing this test");

generate() function which runs binary should be replaced with tst_cmd().
IMHO we don't need to check for $LTPROOT, because we expect PATH to be set
correctly.

>  	}
>  	bin_path = malloc(strlen(ltproot) + 16);
>  	if (bin_path == NULL) {
SAFE_MALLOC() (in many places)
> -		tst_brkm(TBROK | TERRNO, NULL, "malloc failed");
> +		tst_brk(TBROK, "malloc failed");
>  	}
...

> +void run(unsigned int n)
> +{
> +	void *exit_value;
> +	pthread_attr_t newattr;
> +	size_t stacksize = 2093056;
I'm not sure if this is portable for all archs and I'd use #define at the top.

...
> +static struct tst_test test = {
> +	.test = run,
> +	.setup = setup,
> +	.options = opt,
> +	.needs_root = 1,
> +	.needs_tmpdir = 1,
> +	.tcnt = NB_FUNC,
> +};

struct tst_test test should be defined in float*.c tests, not in this
common file included by tests.

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp