Re: [LTP] [PATCH v1] splice06.c: Add splice check on proc files

[Richard Palethorpe <rpalethorpe@suse.de>]

Hello,

Subject had a typo s/splices/splice/.

Wei Gao via ltp <ltp@lists.linux.it> writes:

> Signed-off-by: Wei Gao <wegao@suse.com>
> ---
>  testcases/kernel/syscalls/splice/splice06.c | 134 ++++++++++++++++++++
>  1 file changed, 134 insertions(+)
>  create mode 100644 testcases/kernel/syscalls/splice/splice06.c
>
> diff --git a/testcases/kernel/syscalls/splice/splice06.c b/testcases/kernel/syscalls/splice/splice06.c
> new file mode 100644
> index 000000000..f14fd84c5
> --- /dev/null
> +++ b/testcases/kernel/syscalls/splice/splice06.c
> @@ -0,0 +1,134 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2023 SUSE LLC <wegao@suse.com>
> + */
> +
> +/*\
> + * [Description]
> + *
> + * This test is cover splice() on proc files.
> + *
> + */
> +
> +#define _GNU_SOURCE
> +
> +#include <stdio.h>
> +#include <errno.h>
> +#include <string.h>
> +#include <signal.h>
> +#include <sys/types.h>
> +#include <fcntl.h>
> +
> +#include "tst_test.h"
> +#include "lapi/splice.h"
> +
> +#define TEST_BLOCK_SIZE 100
> +#define NAME_SPACES_NUM 1024
> +#define PROCFILE "/proc/sys/user/max_user_namespaces"

This file is not available on a lot of configs. I'd suggest using
instead /proc/kernel/domainname which lets you read/write up to 64
bytes and is often not set. See kernel/utsname_sysctl.c

We should probably also test an integer based one like
/proc/sys/fs/pipe-max-size.

> +#define TESTFILE1 "splice_testfile_1"
> +#define TESTFILE2 "splice_testfile_2"

Why do we need these files? We should be able to write to a pipe then
splice it to the proc file(s). Splicing to/from a regular file is
handled in other tests.

Also splice from the proc file to a pipe then read the pipe.

> +
> +static int fd_in, fd_out;
> +static int origin_name_spaces_num;
> +static char line[TEST_BLOCK_SIZE];

Why are these global variables?

It's not because you make sure they are closed during cleanup.

> +
> +static void splice_file(void)
> +{
> +	int pipes[2];
> +	int ret;
> +
> +	SAFE_PIPE(pipes);
> +
> +	ret = splice(fd_in, NULL, pipes[1], NULL, TEST_BLOCK_SIZE, 0);
> +	if (ret < 0)
> +		tst_brk(TBROK | TERRNO, "splice(fd_in, pipe) failed");
> +
> +	ret = splice(pipes[0], NULL, fd_out, NULL, TEST_BLOCK_SIZE, 0);
> +	if (ret < 0)
> +		tst_brk(TBROK | TERRNO, "splice(pipe, fd_out) failed");
> +
> +	SAFE_CLOSE(fd_in);
> +	SAFE_CLOSE(fd_out);
> +	SAFE_CLOSE(pipes[0]);
> +	SAFE_CLOSE(pipes[1]);
> +}



> +
> +static void set_value(char file[], int num)
> +{
> +	int fd;
> +	int len = snprintf(NULL, 0, "%d", num);
> +
> +	memset(line, '\0', sizeof(line));
> +	sprintf(line, "%d", num);
> +
> +	fd = SAFE_OPEN(file, O_RDWR | O_CREAT | O_TRUNC, 0777);
> +	SAFE_WRITE(SAFE_WRITE_ALL, fd, line, len);
> +	SAFE_CLOSE(fd);

We have library functions to open then read, scan or write a
file. (SAFE_FILE_*).

> +}
> +
> +static int get_value(char file[])
> +{
> +	int fd, num, ret;
> +
> +	memset(line, '\0', sizeof(line));
> +
> +	fd = SAFE_OPEN(file, O_RDONLY);
> +	SAFE_READ(0, fd, line, TEST_BLOCK_SIZE);
> +	SAFE_CLOSE(fd);
> +
> +	ret = sscanf(line, "%d", &num);
> +	if (ret == EOF)
> +		tst_brk(TBROK | TERRNO, "sscanf error");
> +
> +	return num;
> +}
> +
> +static void splice_test(void)
> +{
> +
> +	int name_spaces_num_setting = get_value(PROCFILE);
> +
> +	fd_in = SAFE_OPEN(PROCFILE, O_RDONLY);
> +	fd_out = SAFE_OPEN(TESTFILE2, O_WRONLY | O_CREAT | O_TRUNC, 0666);
> +	splice_file();

The control flow is unecessarily hard to follow here. You are opening
fds in the outer scope then closing them inside splice_file().

Given that I don't think it is necessary to have TESTFILE1/2 I'll skip
the rest of the function. However you need to make the control flow or
data flow clearer.

> +
> +	if (name_spaces_num_setting == get_value(TESTFILE2))
> +		tst_res(TPASS, "Written data has been read back correctly");
> +	else
> +		tst_brk(TBROK | TERRNO, "Written data has been read back failed");
> +
> +	if (get_value(PROCFILE) != NAME_SPACES_NUM)
> +		name_spaces_num_setting = NAME_SPACES_NUM;
> +	else
> +		name_spaces_num_setting = NAME_SPACES_NUM + 1;
> +
> +	set_value(TESTFILE1, name_spaces_num_setting);
> +
> +	fd_in = SAFE_OPEN(TESTFILE1, O_RDONLY, 0777);
> +	fd_out = SAFE_OPEN(PROCFILE, O_WRONLY, 0777);
> +
> +	splice_file();
> +
> +	if (name_spaces_num_setting == get_value(PROCFILE))
> +		tst_res(TPASS, "Written data has been written correctly");
> +	else
> +		tst_brk(TBROK | TERRNO, "Written data has been written failed");
> +}
> +
> +static void setup(void)
> +{
> +	origin_name_spaces_num = get_value(PROCFILE);

We have a save and restore feature in tst_test (tst_test.save_restore).

> +}
> +
> +static void cleanup(void)
> +{
> +	set_value(PROCFILE, origin_name_spaces_num);
> +}
> +
> +static struct tst_test test = {
> +	.min_kver = "5.11",
> +	.setup = setup,
> +	.cleanup = cleanup,
> +	.test_all = splice_test,
> +	.needs_tmpdir = 1,
> +};
> -- 
> 2.35.3


-- 
Thank you,
Richard.

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