Re: [LTP] [PATCH] Add case about arch_prctl syscall.

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
> +static void run(unsigned int index)
> +{
> +	FILE *fd;

Please do not call FILE * pointer fd, that is confusing since fd is
usually a file descriptor. FILE * pointer is usually just call f.

> +	char needed_flag[] = "cpuid_fault";

I would include the space before flag name just to be extra careful we
do not match anything that has this as a substring. E.g. if in the
future they add something that would be named "no_cpuid_fault" this
would still match strstr(line, "cpuid_fault") but not
strstr(line, " cpuid_fault"); Ideally we would check that there is a
space after the string, but we can't just do that with strstr() since
the flag may be actually last in the list. So the only posibility would
be using the pointer from strstr() and checking that the character after
the matched string is also a space.

> +	char *line = NULL;
> +	size_t len = 0;
> +	bool tag = 0;
> +
> +	fd = SAFE_FOPEN("/proc/cpuinfo", "r");
> +
> +	while (getline(&line, &len, fd) != -1) {
> +		if (strncmp(line, "flag", strlen("flag")) == 0 &&
                                      ^
				      why not just whole "flags" ?
> +				strstr(line, needed_flag) != NULL) {
> +			tag = 1;
> +			break;
> +		}
> +	}

This whole check for flag should be done in the test setup so that we do
not parse the /proc/cpuinfo on each test iteration (-i command line
option).

> +	if (!tag)
> +		tst_brk(TCONF, "CPU need %s flag.", needed_flag);

Why don't we check that the prctl() returns ENODEV when the flag is not
present. That is a valid testcase as well.

> +	// index == 0 for disable cpuid, 1 for enable cpuid.
> +	TST_EXP_PASS(arch_prctl_set(ARCH_SET_CPUID, index));
> +
> +	TEST(arch_prctl_get(ARCH_GET_CPUID));
> +
> +	if (TST_RET == index)
> +		tst_res(TPASS, "get cpuid succeed.");
> +	else
> +		tst_res(TFAIL, "get cpuid failed.");
> +}
> +
> +static struct tst_test test = {
> +	.test = run,
> +	.tcnt = 2,
> +	.min_kver = "4.12",
> +	.supported_archs = (const char *const []){"x86_64", "x86", NULL}
> +};
> +
> +#else /* HAVE_ASM_PRCTL_H */
> +TST_TEST_TCONF("missing <asm/prctl.h>");
> +#endif
> -- 
> 2.39.3
> 

-- 
Cyril Hrubis
chrubis@suse.cz

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