| Message ID | 20260109061716.20258-4-wegao@suse.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | new cmd support option for needs_cmds | expand |
| Context | Check | Description |
|---|---|---|
| ltpci/alpine_latest_gcc | fail | failure |
| ltpci/debian_oldstable_gcc | fail | failure |
| ltpci/debian_stable_gcc | fail | failure |
| ltpci/debian_testing_gcc | fail | failure |
| ltpci/ubuntu_bionic_gcc | fail | failure |
| ltpci/fedora_latest_clang | fail | failure |
| ltpci/debian_testing_clang | fail | failure |
| ltpci/debian_stable_s390x-linux-gnu-gcc_s390x | fail | failure |
| ltpci/debian_stable_aarch64-linux-gnu-gcc_arm64 | fail | failure |
| ltpci/debian_stable_gcc | fail | failure |
| ltpci/debian_stable_powerpc64le-linux-gnu-gcc_ppc64el | fail | failure |
| ltpci/ubuntu_jammy_gcc | fail | failure |
| ltpci/quay-io-centos-centos_stream9_gcc | fail | failure |
| ltpci/debian_oldstable_clang | fail | failure |
| ltpci/opensuse-leap_latest_gcc | fail | failure |
| ltpci/opensuse-archive_42-2_gcc | fail | failure |
> TST_ASSERT_INT(partscan_path, 0); > TST_ASSERT_INT(autoclear_path, 0); > TST_ASSERT_STR(backing_path, backing_file_path); > @@ -114,10 +97,23 @@ static void verify_ioctl_loop(void) > > static void setup(void) > { > + parted_sup = tst_cmd_present("parted"); In the logic of tst_test.c, there already did check and store the value in tst_test->needs_cmds.present, so why here do the check seperately again? + struct tst_cmd *pcmd = tst_test->needs_cmds; + while (pcmd->cmd) { + pcmd->present = tst_check_cmd(pcmd->cmd, !pcmd->optional) ? 1 : 0; + pcmd++; + } > + const char *const cmd_parted[] = {"parted", "-s", dev_path, "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); > + > + tst_attach_device(dev_path, "test.img"); > + attach_flag = 1; > + > + if (parted_sup) Can we just reuse the 'tst_test->needs_cmds.present' result? > + SAFE_CMD(cmd_parted, NULL, NULL);
On Fri, Jan 16, 2026 at 09:25:05PM +0800, Li Wang wrote: > > TST_ASSERT_INT(partscan_path, 0); > > TST_ASSERT_INT(autoclear_path, 0); > > TST_ASSERT_STR(backing_path, backing_file_path); > > @@ -114,10 +97,23 @@ static void verify_ioctl_loop(void) > > > > static void setup(void) > > { > > + parted_sup = tst_cmd_present("parted"); > > > In the logic of tst_test.c, there already did check and store the > value in tst_test->needs_cmds.present, so why here do the check > seperately again? > > + struct tst_cmd *pcmd = tst_test->needs_cmds; > + while (pcmd->cmd) { > + pcmd->present = tst_check_cmd(pcmd->cmd, > !pcmd->optional) ? 1 : 0; > + pcmd++; > + } > Let me clarify, and please correct me if I’m mistaken :) tst_cmd_present not do tst_check_cmd again but just go through list find specific cmd such as "parted" present status. > > > + const char *const cmd_parted[] = {"parted", "-s", dev_path, "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); > > + > > + tst_attach_device(dev_path, "test.img"); > > + attach_flag = 1; > > + > > + if (parted_sup) > > Can we just reuse the 'tst_test->needs_cmds.present' result? > You cannot access tst_test->needs_cmds.present directly as if it were a single variable, because needs_cmds is an array (a list) of structures. To get the status of a specific command like "parted", you must iterate through that list to find the entry matching that name. > > + SAFE_CMD(cmd_parted, NULL, NULL); > > > -- > Regards, > Li Wang >
Wei Gao <wegao@suse.com> wrote: > > > Can we just reuse the 'tst_test->needs_cmds.present' result? > > > You cannot access tst_test->needs_cmds.present directly as if it were a > single variable, > because needs_cmds is an array (a list) of structures. To get the status > of a specific command > like "parted", you must iterate through that list to find the entry > matching that name. > So how about using: tst_test->needs_cmds[0].present?
On Mon, Jan 19, 2026 at 11:00:07AM +0800, Li Wang wrote: > Wei Gao <wegao@suse.com> wrote: > > > > > > > Can we just reuse the 'tst_test->needs_cmds.present' result? > > > > > You cannot access tst_test->needs_cmds.present directly as if it were a > > single variable, > > because needs_cmds is an array (a list) of structures. To get the status > > of a specific command > > like "parted", you must iterate through that list to find the entry > > matching that name. > > > > So how about using: tst_test->needs_cmds[0].present? Yes, i think direct indexing is more efficient, but helper function is more readable and extensible(If someone add more entry into .needs_cmds it possible break the test case). I would like to get more feedback on this topic before next patch. @Petr @Cyril ? > > > -- > Regards, > Li Wang
Wei Gao <wegao@suse.com> wrote: > > > > Can we just reuse the 'tst_test->needs_cmds.present' result? > > > > > > > You cannot access tst_test->needs_cmds.present directly as if it were a > > > single variable, > > > because needs_cmds is an array (a list) of structures. To get the status > > > of a specific command > > > like "parted", you must iterate through that list to find the entry > > > matching that name. > > > > > > > So how about using: tst_test->needs_cmds[0].present? > Yes, i think direct indexing is more efficient, but helper function is more readable > and extensible(If someone add more entry into .needs_cmds it possible break the > test case). Yes, but not entirely, once we have the function tst_cmd_present(), we need to manually write the "commands" again, and easy to typo. Even if we plan to add this feature, perhaps it's better not to place it in tst_test.h or tst_test.c, as that may not be the best fit. We should respect the principle of high cohesion and low coupling.
Hi!
Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
Hi!
Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
diff --git a/testcases/kernel/syscalls/ioctl/ioctl_loop01.c b/testcases/kernel/syscalls/ioctl/ioctl_loop01.c index 9fbdbb1f2..b8df2d633 100644 --- a/testcases/kernel/syscalls/ioctl/ioctl_loop01.c +++ b/testcases/kernel/syscalls/ioctl/ioctl_loop01.c @@ -79,23 +79,6 @@ static void check_loop_value(int set_flag, int get_flag, int autoclear_field) static void verify_ioctl_loop(void) { - int ret; - 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"); - - 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); - - attach_flag = 1; - TST_ASSERT_INT(partscan_path, 0); TST_ASSERT_INT(autoclear_path, 0); TST_ASSERT_STR(backing_path, backing_file_path); @@ -114,10 +97,23 @@ static void verify_ioctl_loop(void) static void setup(void) { + parted_sup = tst_cmd_present("parted"); + + const char *const cmd_parted[] = {"parted", "-s", dev_path, "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); + + tst_attach_device(dev_path, "test.img"); + attach_flag = 1; + + if (parted_sup) + SAFE_CMD(cmd_parted, NULL, NULL); + 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); @@ -149,5 +145,9 @@ static struct tst_test test = { {"linux-git", "6ac92fb5cdff"}, {} }, + .needs_cmds = (struct tst_cmd[]) { + {.cmd = "parted", .optional = 1}, + {} + }, .needs_tmpdir = 1, };