Re: [LTP] [PATCH v2 3/3] cgroups: Add first IO controller test

[Richard Palethorpe <rpalethorpe@suse.de>]

Hello Petr,

Petr Vorel <pvorel@suse.cz> writes:

> Hi Richie,
>
>> +++ b/testcases/kernel/controllers/io/io_control01.c
>> @@ -0,0 +1,155 @@
>> +// SPDX-License-Identifier: GPL-2.0
> nit: not sure if it was deliberate not adding it, but you may want to
> add your/SUSE copyright.
>
>> +/*\
>> + *
>> + * [Description]
>> + *
>> + * Perform some I/O on a file and check if at least some of it is
>> + * recorded by the I/O controller.
>> + *
>> + * The exact amount of I/O performed is dependent on the file system,
>> + * page cache, scheduler and block driver. We call sync and drop the
>> + * file's page cache to force reading and writing. We also write
>> + * random data to try to prevent compression.
>> + *
>> + * The pagecache is a particular issue for reading. If the call to
>> + * fadvise is ignored then the data may only be read from the
>> + * cache. So that no I/O requests are made.
>> + */
>> +
> ...
>> +static int read_io_stats(const char *const line, struct io_stats *const stat)
>> +{
>> +	return sscanf(line,
>> +		      "%u:%u rbytes=%lu wbytes=%lu rios=%lu wios=%lu dbytes=%lu dios=%lu",
>> +		      &stat->mjr, &stat->mnr,
>> +		      &stat->rbytes, &stat->wbytes, &stat->rios, &stat->wios,
>> +		      &stat->dbytes, &stat->dios);
>> +}
> checkpatch.pl false positive:
> io_control01.c:40: WARNING: unchecked sscanf return value
> Obviously perl parsing has some limitations as we check read_io_stats() return
> value.

I'm not sure what to do about this other than switch to a macro which is
a bit silly.

IMO sscanf should have the warn_unused_result attribute and this should
be inherited by read_io_stats. All of which is better handled by the
compiler.

>
> ...
>> +static void setup(void)
>> +{
>> +	char buf[PATH_MAX] = { 0 };
>> +	char *path = SAFE_GETCWD(buf, PATH_MAX - sizeof("mnt") - 1);
>> +	struct stat st;
>> +
>> +	strcpy(path + strlen(path), "/mnt");
>> +
>> +	tst_stat_mount_dev(path, &st);
>> +	dev_major = major(st.st_rdev);
>> +	dev_minor = minor(st.st_rdev);
>> +}
>> +
>> +static struct tst_test test = {
>> +	.test_all = run,
>> +	.setup = setup,
>> +	.needs_device = 1,
> nit: testcases/kernel/controllers/io/io_control01.c: useless tag: needs_device

Pushed with fixes (including needs_device).
-- 
Thank you,
Richard.

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