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

["xuyang2018.jy@fujitsu.com" <xuyang2018.jy@fujitsu.com>]

Hi Petr
> 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.
I prefer to detect it in /proc/modules.
What do you think about it?
>
> 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?
OK.
>
> ...
>
>>   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");
of course.
>
> 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).
I prefer to ignore it.
>
> ...
>> +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).
OK.
>> +	if (errno == ENOENT)
>> +		SAFE_CMD(cmd_modprobe, NULL, NULL);
>> +
>> +	modprobe = 1;
>> +}
>
> Kind regards,
> Petr

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