Re: [LTP] [PATCH v2] ioctl_loop01.c: Use proper device for partitioning

[Petr Vorel <pvorel@suse.cz>]

Hi Wei,

> This is same patch used on ioctl09,the page cache of loop0 can cache old
> version of the partition table which is then used by the partitioning
> code. Fix the problem by calling parted against the loop device directly.

LGTM, thanks!

Reviewed-by: Petr Vorel <pvorel@suse.cz>

> Link: https://lore.kernel.org/ltp/20250829141932.31997-1-jack@suse.cz/
very nit: Link in kernel commits (and I use this approach) is used for the link
to the patch which was merged (kernel maintainer or here LTP maintainer adds
it).  Linking everything else I would use [1].

> Signed-off-by: Wei Gao <wegao@suse.com>
> ---
>  .../kernel/syscalls/ioctl/ioctl_loop01.c      | 26 +++++--------------
>  1 file changed, 7 insertions(+), 19 deletions(-)

> diff --git a/testcases/kernel/syscalls/ioctl/ioctl_loop01.c b/testcases/kernel/syscalls/ioctl/ioctl_loop01.c
> index c9137bf1e..695aaeb0b 100644
> --- a/testcases/kernel/syscalls/ioctl/ioctl_loop01.c
> +++ b/testcases/kernel/syscalls/ioctl/ioctl_loop01.c
> @@ -78,7 +78,13 @@ static void check_loop_value(int set_flag, int get_flag, int autoclear_field)

>  static void verify_ioctl_loop(void)
>  {
> +	const char *const cmd_parted[] = {"parted", "-s", dev_path, "mklabel", "msdos", "mkpart",
> +					"primary", "ext4", "1M", "10M", NULL};
> +
> +	tst_fill_file("test.img", 0, 1024 * 1024, 10);
>  	tst_attach_device(dev_path, "test.img");
> +	SAFE_CMD(cmd_parted, NULL, NULL);

I'll would probably keep the previous approach where the rest of the testing is
now quited:

Now:
...
tst_cmd.c:65: TCONF: Couldn't find 'parted' in $PATH at tst_cmd.c:65

Summary:
passed   0
failed   0
broken   0
skipped  1
warnings 0

Previously:
ioctl_loop01.c:116: TCONF: parted binary not installed or failed
tst_buffers.c:57: TINFO: Test is using guarded buffers
ioctl_loop01.c:84: TPASS: /sys/block/loop2/loop/partscan = 0
ioctl_loop01.c:85: TPASS: /sys/block/loop2/loop/autoclear = 0
ioctl_loop01.c:86: TPASS: /sys/block/loop2/loop/backing_file = '/tmp/LTP_ioco0FWzP/test.img'
ioctl_loop01.c:56: TPASS: get expected lo_flag 12
ioctl_loop01.c:58: TPASS: /sys/block/loop2/loop/partscan = 1
ioctl_loop01.c:59: TPASS: /sys/block/loop2/loop/autoclear = 1
ioctl_loop01.c:62: TINFO: Current environment doesn't have parted disk, skip it
ioctl_loop01.c:90: TINFO: Test flag can be clear
ioctl_loop01.c:56: TPASS: get expected lo_flag 8
ioctl_loop01.c:58: TPASS: /sys/block/loop2/loop/partscan = 1
ioctl_loop01.c:59: TPASS: /sys/block/loop2/loop/autoclear = 0
ioctl_loop01.c:62: TINFO: Current environment doesn't have parted disk, skip it

Summary:
passed   9
failed   0
broken   0
skipped  1
warnings 0

My point was to add TST_CMD_TCONF_ON_MISSING to the previous call. But I was
wrong, that would work the same (skip whole testing on missing 'parted').

...
>  static void setup(void)
>  {
> -	int ret;
> -	const char *const cmd_parted[] = {"parted", "-s", "test.img", "mklabel", "msdos", "mkpart",
> -	                                  "primary", "ext4", "1M", "10M", NULL};
> -
>  	dev_num = tst_find_free_loopdev(dev_path, sizeof(dev_path));
>  	if (dev_num < 0)
>  		tst_brk(TBROK, "Failed to find free loop device");

> -	tst_fill_file("test.img", 0, 1024 * 1024, 10);
> -
> -	ret = tst_cmd(cmd_parted, NULL, NULL, TST_CMD_PASS_RETVAL);
> -	switch (ret) {
> -	case 0:
> -		parted_sup = 1;
> -	break;
> -	case 255:
> -		tst_res(TCONF, "parted binary not installed or failed");
> -	break;
> -	default:
> -		tst_res(TCONF, "parted exited with %i", ret);
> -	break;

Therefore either keeping this, or use if/else equivalent (IMHO more readable):

	ret = tst_cmd(cmd_parted, NULL, NULL, TST_CMD_PASS_RETVAL);
	if (!ret)
		parted_sup = 1;
	else if (ret == 255)
		tst_res(TCONF, "parted binary not installed or failed");
	else
		tst_res(TCONF, "parted exited with %i", ret);

Kind regards,
Petr

> -	}
> -
>  	sprintf(partscan_path, "/sys/block/loop%d/loop/partscan", dev_num);
>  	sprintf(autoclear_path, "/sys/block/loop%d/loop/autoclear", dev_num);
>  	sprintf(backing_path, "/sys/block/loop%d/loop/backing_file", dev_num);

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