Re: [LTP] [PATCH v1] io_submit05.c: Add test case for RWF_APPEND flag

[Petr Vorel <pvorel@suse.cz>]

Hi Wei,

> +++ b/runtest/syscalls
> @@ -656,6 +656,7 @@ io_setup02 io_setup02
>  io_submit01 io_submit01
>  io_submit02 io_submit02
>  io_submit03 io_submit03
> +io_submit05 io_submit05
nit: you have io_submit04 patchset, that's why 04 is skipped. You send them
separately, this can lead to unapplicable patch. As we also discussed 04, could
you send them both in one patchset (even they are separate), because they
influence each other as they touch bouth runtest/syscalls and .gitignore on the
same place?

...
>  keyctl01 keyctl0> +++ b/testcases/kernel/syscalls/io_submit/io_submit05.c
> @@ -0,0 +1,123 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2023 Wei Gao <wegao@suse.com>
> + */
> +
> +/*\
> + * [Description]
> + *
> + * This is a basic test for io_submit RWF_APPEND flag.
                                       ^ nit: io_submit()
> + *
> + */
> +
> +#include <linux/aio_abi.h>
> +
> +#include "config.h"
We don't need config.h, right?

> +#include "tst_test.h"
> +#include "tst_safe_pthread.h"
And we don't need this header either.

> +#include "lapi/syscalls.h"
...

> +static char *init_buf;
> +static char *update_buf;
> +static char *tmp_buf;

> +static inline void io_prep_option(struct iocb *cb, int fd, void *buf,
> +			size_t count, long long offset, unsigned int opcode)
> +{
> +	memset(cb, 0, sizeof(*cb));
You don't need to do this, cb is static struct iocb iocb, which is {} because static.

> +	cb->aio_fildes = fd;
> +	cb->aio_lio_opcode = opcode;
> +	cb->aio_buf = (uint64_t)buf;
> +	cb->aio_offset = offset;
> +	cb->aio_nbytes = count;
> +	cb->aio_rw_flags = RWF_APPEND;
> +}
Also, I don't see much point of having this as a separate function, I would just
put this in the end of setup().

> +static unsigned int io_submit(void)
As Andrea mentioned, this should be void. But I also does not like the name...
> +{
> +	struct io_event evbuf;
Although we use only one array thus it works, man says: struct iocb **iocbpp),
thus it should have been:
struct io_event evbuf[1] = {};

Newer mind, but I would probably initialize (although it should not be needed as
we pass it to kernel thus other tests does not do it, it's a good habit):
struct io_event evbuf = {};

> +	struct timespec timeout = { .tv_sec = 1 };
> +
> +	TST_EXP_VAL_SILENT(tst_syscall(__NR_io_submit, ctx, 1, iocbs), 1);
We probably want to quit testing if __NR_io_submit fails (probably
tst_brk(TBROK)).

> +	TST_EXP_VAL_SILENT(tst_syscall(__NR_io_getevents, ctx, 1, 1, &evbuf,
> +			&timeout), 1);

Although we don't use libaio wrapper it's a bit confusing we have io_submit()
function which is not just __NR_io_submit wrapper, but call also
__NR_io_getevents. Maybe use a different name? Or, maybe, add these 4 lines into
the main test function?

Maybe also verify evbuf.res == BUF_LEN ? E.g.:

TST_EXP_VAL_SILENT(evbuf.res, BUF_LEN);

> +}

> +
> +static void setup(void)
> +{
> +
> +	TST_EXP_PASS_SILENT(tst_syscall(__NR_io_setup, 1, &ctx));
> +
> +	fd = SAFE_OPEN(TEST_FILE, O_RDWR | O_CREAT, MODE);
> +	SAFE_LSEEK(fd, 0, SEEK_SET);
> +
> +	memset(init_buf, 0x62, BUF_LEN);

> +	memset(update_buf, 0x61, BUF_LEN);
I would also use 'a' and 'b' values (slightly more readable).
And 'a' is what is going to be tested, maybe #define it at the top
(readability, as you define other constants)?

We don't need this memset() either (static variable).
But we want to memset() update_buf each run (for -iN), right?
Then in should be in the test function (again setup() is run only once,
regardless of N in -iN).

> +	memset(tmp_buf, 0, BUF_LEN);
The same about memset() aplies to tmp_buf.

> +
> +	io_prep_option(&iocb, fd, update_buf, BUF_LEN, 1, IOCB_CMD_PWRITE);
> +}
> +
> +static void cleanup(void)
> +{
> +	if (fd > 0)
> +		SAFE_CLOSE(fd);
> +
> +	if (tst_syscall(__NR_io_destroy, ctx))
> +		tst_brk(TBROK | TERRNO, "io_destroy() failed");
> +}
> +
> +
> +static void run(void)
> +{
> +	int i;
> +
> +	SAFE_WRITE(0, fd, init_buf, BUF_LEN);
> +	io_submit();
> +	SAFE_LSEEK(fd, BUF_LEN, SEEK_SET);
> +	SAFE_READ(1, fd, tmp_buf, BUF_LEN);
very nit: space before for helps readability.
> +	for (i = 0; i < BUF_LEN; i++) {
> +		if (tmp_buf[i] != 0x61)
Why not here print tst_res(TFAIL) and return, instead of break?
> +			break;
> +	}
> +
> +	if (i != BUF_LEN) {
> +		tst_res(TFAIL, "buffer wrong at %i have %c expected 'a'",
Here you hardwire 'a', which elsewhere you reference on other places as 0x61.
Again use single definition would be better.

Kind regards,
Petr

> +				i, tmp_buf[i]);
> +		return;
> +	}
> +
> +	tst_res(TPASS, "io_submit wrote %zi bytes successfully "
> +			"with content 'a' expectedly ", BUF_LEN);
> +}
> +
> +static struct tst_test test = {
> +	.needs_tmpdir = 1,
> +	.needs_kconfigs = (const char *[]) {
> +		"CONFIG_AIO=y",
> +		NULL
> +	},
> +	.setup = setup,
> +	.cleanup = cleanup,
> +	.test_all = run,
> +	.max_runtime = 60,
> +	.mntpoint = MNTPOINT,
> +	.mount_device = 1,
> +	.all_filesystems = 1,
> +	.bufs = (struct tst_buffers []) {
> +		{&init_buf, .size = BUF_LEN},
> +		{&update_buf, .size = BUF_LEN},
> +		{&tmp_buf, .size = BUF_LEN},
> +		{}
> +	}
> +};

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