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

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

Hi Petr
> 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?
Since we have require modprobe/rmmod commands, using lsmod is better. I 
will use lsmod as you suggested.
.>>
>> 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