[LTP] [PATCH v2] syscalls/ioctl09: Add test for BLKRRPART ioctl syscalls/ioctl09: Add test for BLKRRPART ioctl

[Cyril Hrubis]

Hi!
> Fixes #699

Looks good a couple of minor comments below.

> Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com>
> Acked-by: Jan Stancek <jstancek@redhat.com>
> ---
> v1->v2:
> code style fix(below 80 characters)
>  runtest/syscalls                           |   1 +
>  testcases/kernel/syscalls/ioctl/.gitignore |   1 +
>  testcases/kernel/syscalls/ioctl/ioctl09.c  | 126 +++++++++++++++++++++
>  3 files changed, 128 insertions(+)
>  create mode 100644 testcases/kernel/syscalls/ioctl/ioctl09.c
> 
> diff --git a/runtest/syscalls b/runtest/syscalls
> index 5b3a0862f..aaa81e4ee 100644
> --- a/runtest/syscalls
> +++ b/runtest/syscalls
> @@ -529,6 +529,7 @@ ioctl05      ioctl05
>  ioctl06      ioctl06
>  ioctl07      ioctl07
>  ioctl08      ioctl08
> +ioctl09      ioctl09
>  
>  ioctl_loop01 ioctl_loop01
>  ioctl_loop02 ioctl_loop02
> diff --git a/testcases/kernel/syscalls/ioctl/.gitignore b/testcases/kernel/syscalls/ioctl/.gitignore
> index 3a3d49adc..5fff7a61d 100644
> --- a/testcases/kernel/syscalls/ioctl/.gitignore
> +++ b/testcases/kernel/syscalls/ioctl/.gitignore
> @@ -6,6 +6,7 @@
>  /ioctl06
>  /ioctl07
>  /ioctl08
> +/ioctl09
>  /ioctl_loop01
>  /ioctl_loop02
>  /ioctl_loop03
> diff --git a/testcases/kernel/syscalls/ioctl/ioctl09.c b/testcases/kernel/syscalls/ioctl/ioctl09.c
> new file mode 100644
> index 000000000..b39ef9874
> --- /dev/null
> +++ b/testcases/kernel/syscalls/ioctl/ioctl09.c
> @@ -0,0 +1,126 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2020 FUJITSU LIMITED. All rights reserved.
> + * Author: Yang Xu <xuyang2018.jy@cn.jujitsu.com>
> + *
> + * Basic test for the BLKRRPART ioctl, it is the same as blockdev
> + * --rereadpt command.
> + */
> +
> +#include <stdio.h>
> +#include <unistd.h>
> +#include <string.h>
> +#include <sys/mount.h>
> +#include <stdbool.h>
> +#include "lapi/loop.h"
> +#include "tst_test.h"
> +
> +static char dev_path[1024];
> +static int dev_num, attach_flag, dev_fd;
> +static char loop_partpath[1026], sys_loop_partpath[1026];
> +
> +static void change_partition(const char *const cmd[])
> +{
> +	int ret;
> +
> +	ret = tst_cmd(cmd, NULL, NULL, TST_CMD_PASS_RETVAL);
> +	switch (ret) {
> +	case 0:
> +	break;
> +	case 255:
> +		tst_res(TCONF, "parted binary not installed or failed");
> +	break;

We do have .needs_cmds in the test structure so we don't have to handle
255 here, the test will not start if parted is not installed.

> +	default:
> +		tst_res(TCONF, "parted exited with %i", ret);

Shouldn't this be TBROK?

Or at least tst_brk() because we will proceed with the test as it is and
possibly fail the test since parted haven't modified the binary. Or does
parted return non-zero when it succeeds?

Generally if we are going to handle only one failure case we can write it as:

	ret = tst_cmd(...);
	if (ret)
		tst_brk(TBROK, "parted returned %i", ret);

> +	break;
> +	}
> +}
> +
> +static void check_partition(int part_num, bool value)
> +{
> +	int ret;
> +
> +	sprintf(sys_loop_partpath, "/sys/block/loop%d/loop%dp%d",
> +		dev_num, dev_num, part_num);
> +	sprintf(loop_partpath, "%sp%d", dev_path, part_num);
> +
> +	ret = access(sys_loop_partpath, F_OK);
> +	if (ret == 0)
> +		tst_res(value ? TPASS : TFAIL, "access %s succeeds",
> +			sys_loop_partpath);
> +	else
> +		tst_res(value ? TFAIL : TPASS, "access %s fails",
> +			sys_loop_partpath);
> +
> +	ret = access(loop_partpath, F_OK);
> +	if (ret == 0)
> +		tst_res(value ? TPASS : TFAIL, "access %s succeeds",
> +			loop_partpath);
> +	else
> +		tst_res(value ? TFAIL : TPASS, "access %s fails",
> +			loop_partpath);
> +}
> +
> +static void verify_ioctl(void)
> +{
> +	const char *const cmd_parted_old[] = {"parted", "-s", "test.img",
> +					      "mklabel", "msdos", "mkpart",
> +					      "primary", "ext4", "1M", "10M",
> +					      NULL};
> +	const char *const cmd_parted_new[] = {"parted", "-s", "test.img",
> +					      "mklabel", "msdos", "mkpart",
> +					      "primary", "ext4", "1M", "10M",
> +					      "mkpart", "primary", "ext4",
> +					      "10M", "20M", NULL};
> +	struct loop_info loopinfo = {0};
> +
> +	dev_num = tst_find_free_loopdev(dev_path, sizeof(dev_path));
> +	if (dev_num < 0)
> +		tst_brk(TBROK, "Failed to find free loop device");

Shouldn't we move the tst_find_free_loopdev() to the test setup?

> +	tst_fill_file("test.img", 0, 1024 * 1024, 20);

I wonder if the recently introduced tst_prealloc_file() would make the
test a bit faster. Have you tried that?

> +	change_partition(cmd_parted_old);
> +
> +	tst_attach_device(dev_path, "test.img");
> +	attach_flag = 1;
> +
> +	dev_fd = SAFE_OPEN(dev_path, O_RDWR);
> +	loopinfo.lo_flags =  LO_FLAGS_PARTSCAN;
> +	SAFE_IOCTL(dev_fd, LOOP_SET_STATUS, &loopinfo);
> +	check_partition(1, true);
> +	check_partition(2, false);
> +
> +	change_partition(cmd_parted_new);
> +	TST_RETRY_FUNC(ioctl(dev_fd, BLKRRPART, 0), TST_RETVAL_EQ0);
> +	check_partition(1, true);
> +	check_partition(2, true);
> +
> +	SAFE_CLOSE(dev_fd);
> +	tst_detach_device(dev_path);
> +	attach_flag = 0;
> +	unlink("test.img");
> +}
> +
> +static void cleanup(void)
> +{
> +	if (dev_fd > 0)
> +		SAFE_CLOSE(dev_fd);
> +	if (attach_flag)
> +		tst_detach_device(dev_path);
> +}
> +
> +static struct tst_test test = {
> +	.cleanup = cleanup,
> +	.test_all = verify_ioctl,
> +	.needs_root = 1,
> +	.needs_drivers = (const char *const []) {
> +		"loop",
> +		NULL
> +	},
> +	.needs_cmds = (const char *const []) {
> +		"parted",
> +		NULL
> +	},
> +	.needs_tmpdir = 1,
> +};
> -- 
> 2.23.0
> 
> 
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz