Re: [LTP] [PATCH 3/3] zram/zram03: Convert into new api

[Petr Vorel <pvorel@suse.cz>]

Hi Xu,

> Also add removing zram module step in setup, so we can avoid the situation that
> zram module is being used by zram-generator.

...
> +++ b/testcases/kernel/device-drivers/zram/zram03.c
...
> +static const char *const cmd_rmmod[] = {"rmmod", "zram", NULL};

Again, here should be lsmod used.

Except this LGTM.
Reviewed-by: Petr Vorel <pvorel@suse.cz>


>  static void set_disksize(void)
>  {
> -	tst_resm(TINFO, "create a zram device with %ld bytes in size.", SIZE);
> -	SAFE_FILE_PRINTF(cleanup, PATH_ZRAM "/disksize", "%ld", SIZE);
> +	tst_res(TINFO, "create a zram device with %ld bytes in size.", SIZE);
> +	SAFE_FILE_PRINTF(PATH_ZRAM "/disksize", "%ld", SIZE);
>  }

>  static void write_device(void)
> @@ -92,17 +45,16 @@ static void write_device(void)
>  	int fd;
>  	char *s;

> -	tst_resm(TINFO, "map this zram device into memory.");
> -	fd = SAFE_OPEN(cleanup, DEVICE, O_RDWR);
> -	s = SAFE_MMAP(cleanup, NULL, SIZE, PROT_READ | PROT_WRITE,
> -		      MAP_SHARED, fd, 0);
> +	tst_res(TINFO, "map this zram device into memory.");
> +	fd = SAFE_OPEN(DEVICE, O_RDWR);
> +	s = SAFE_MMAP(NULL, SIZE, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);

> -	tst_resm(TINFO, "write all the memory.");
> +	tst_res(TINFO, "write all the memory.");
very nit: while at it, could you please remove unnecessary dot at the end?

...

>  static void reset(void)
>  {
> -	tst_resm(TINFO, "reset it.");
> -	SAFE_FILE_PRINTF(cleanup, PATH_ZRAM "/reset", "1");
> -}
...
> +	tst_res(TINFO, "%s...", __func__);
> +	SAFE_FILE_PRINTF(PATH_ZRAM "/reset", "1");
>  }

checkpatch.pl complains:

$ make check-zram03
zram03.c:93: WARNING: Unnecessary ftrace-like logging - prefer using ftrace
=> nit, but checkpatch.pl is correct, previous info was better than tracing
function name. How about keep info instead of function name, e.g.:
tst_res(TINFO, "reset zram");

zram03.c:94: WARNING: Prefer using '"%s...", __func__' to using 'reset', this function's name, in a string
false positive (not sure if we should bother with renaming function to keep it quiet).

...
> +static void setup(void)
> +{
> +	const char *const cmd_modprobe[] = {"modprobe", "zram", NULL};
> +
> +	if (tst_cmd(cmd_rmmod, NULL, NULL, TST_CMD_PASS_RETVAL)) {
> +		if (errno == EBUSY)
> +			tst_brk(TCONF, "zram module may being used!");
> +	}
nit: I'd add space here for a readability (it's on more places).
> +	if (errno == ENOENT)
> +		SAFE_CMD(cmd_modprobe, NULL, NULL);
> +
> +	modprobe = 1;
> +}

Kind regards,
Petr

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