| Message ID | 20230302014519.31512-1-wegao@suse.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v7] fsconfig03: SKIP check return value for old kernel | expand |
Hi Wei, > Signed-off-by: Wei Gao <wegao@suse.com> > --- > .../kernel/syscalls/fsconfig/fsconfig03.c | 21 ++++++++++++------- > 1 file changed, 13 insertions(+), 8 deletions(-) > diff --git a/testcases/kernel/syscalls/fsconfig/fsconfig03.c b/testcases/kernel/syscalls/fsconfig/fsconfig03.c > index 7ee37f4ae..9adf06207 100644 > --- a/testcases/kernel/syscalls/fsconfig/fsconfig03.c > +++ b/testcases/kernel/syscalls/fsconfig/fsconfig03.c > @@ -41,15 +41,20 @@ static void run(void) > if (pagesize == -1) > tst_brk(TBROK, "sysconf(_SC_PAGESIZE) failed"); > - for (size_t i = 0; i < 5000; i++) { > - /* use same logic in kernel legacy_parse_param function */ > - const size_t len = i * (strlen(val) + 2) + (strlen(val) + 1) + 2; > + if ((tst_kvercmp(5, 17, 1)) >= 0) { I suppose 722d94847de29 (in .tags) change the old behavior (from v5.17-rc1). Shouldn't be the check against 5.17.0? if ((tst_kvercmp(5, 17, 0)) >= 0) { > + for (size_t i = 0; i < 5000; i++) { > + /* use same logic in kernel legacy_parse_param function */ > + const size_t len = i * (strlen(val) + 2) + (strlen(val) + 1) + 2; > - if (!strcmp(tst_device->fs_type, "btrfs") && len <= (size_t)pagesize) > - TST_EXP_PASS_SILENT(fsconfig(fd, FSCONFIG_SET_STRING, "\x00", val, 0)); > - else > - TST_EXP_FAIL_SILENT(fsconfig(fd, FSCONFIG_SET_STRING, "\x00", val, 0), > - EINVAL); > + if (!strcmp(tst_device->fs_type, "btrfs") && len <= (size_t)pagesize) > + TST_EXP_PASS_SILENT(fsconfig(fd, FSCONFIG_SET_STRING, "\x00", val, 0)); > + else > + TST_EXP_FAIL_SILENT(fsconfig(fd, FSCONFIG_SET_STRING, "\x00", val, 0), > + EINVAL); > + } > + } else { > + for (size_t i = 0; i < 5000; i++) Repeating the loop again. Wouldn't be more readable moving the if clause to separate function and doing if/else inside of for loop? Also "\x00" might be in #define (used 3 times). Kind regards, Petr > + fsconfig(fd, FSCONFIG_SET_STRING, "\x00", val, 0); > }
Hi Wei, ... > + } else { > + for (size_t i = 0; i < 5000; i++) > + fsconfig(fd, FSCONFIG_SET_STRING, "\x00", val, 0); Why calling fsconfig() without any check for old kernels? It really looks strange we don't care about the result. That would deserve an explanation in the commit message. Kind regards, Petr
On Thu, Mar 02, 2023 at 11:00:08AM +0100, Petr Vorel wrote: > Hi Wei, > > > Signed-off-by: Wei Gao <wegao@suse.com> > > --- > > .../kernel/syscalls/fsconfig/fsconfig03.c | 21 ++++++++++++------- > > 1 file changed, 13 insertions(+), 8 deletions(-) > > > diff --git a/testcases/kernel/syscalls/fsconfig/fsconfig03.c b/testcases/kernel/syscalls/fsconfig/fsconfig03.c > > index 7ee37f4ae..9adf06207 100644 > > --- a/testcases/kernel/syscalls/fsconfig/fsconfig03.c > > +++ b/testcases/kernel/syscalls/fsconfig/fsconfig03.c > > @@ -41,15 +41,20 @@ static void run(void) > > if (pagesize == -1) > > tst_brk(TBROK, "sysconf(_SC_PAGESIZE) failed"); > > > - for (size_t i = 0; i < 5000; i++) { > > - /* use same logic in kernel legacy_parse_param function */ > > - const size_t len = i * (strlen(val) + 2) + (strlen(val) + 1) + 2; > > + if ((tst_kvercmp(5, 17, 1)) >= 0) { > I suppose 722d94847de29 (in .tags) change the old behavior (from v5.17-rc1). > Shouldn't be the check against 5.17.0? > if ((tst_kvercmp(5, 17, 0)) >= 0) { yes, 5.17.0 is better one! > > > > + for (size_t i = 0; i < 5000; i++) { > > + /* use same logic in kernel legacy_parse_param function */ > > + const size_t len = i * (strlen(val) + 2) + (strlen(val) + 1) + 2; > > > - if (!strcmp(tst_device->fs_type, "btrfs") && len <= (size_t)pagesize) > > - TST_EXP_PASS_SILENT(fsconfig(fd, FSCONFIG_SET_STRING, "\x00", val, 0)); > > - else > > - TST_EXP_FAIL_SILENT(fsconfig(fd, FSCONFIG_SET_STRING, "\x00", val, 0), > > - EINVAL); > > + if (!strcmp(tst_device->fs_type, "btrfs") && len <= (size_t)pagesize) > > + TST_EXP_PASS_SILENT(fsconfig(fd, FSCONFIG_SET_STRING, "\x00", val, 0)); > > + else > > + TST_EXP_FAIL_SILENT(fsconfig(fd, FSCONFIG_SET_STRING, "\x00", val, 0), > > + EINVAL); > > + } > > + } else { > > + for (size_t i = 0; i < 5000; i++) > Repeating the loop again. Wouldn't be more readable moving the if clause to > separate function and doing if/else inside of for loop? > > Also "\x00" might be in #define (used 3 times). > > Kind regards, > Petr > > > + fsconfig(fd, FSCONFIG_SET_STRING, "\x00", val, 0); > > }
diff --git a/testcases/kernel/syscalls/fsconfig/fsconfig03.c b/testcases/kernel/syscalls/fsconfig/fsconfig03.c index 7ee37f4ae..9adf06207 100644 --- a/testcases/kernel/syscalls/fsconfig/fsconfig03.c +++ b/testcases/kernel/syscalls/fsconfig/fsconfig03.c @@ -41,15 +41,20 @@ static void run(void) if (pagesize == -1) tst_brk(TBROK, "sysconf(_SC_PAGESIZE) failed"); - for (size_t i = 0; i < 5000; i++) { - /* use same logic in kernel legacy_parse_param function */ - const size_t len = i * (strlen(val) + 2) + (strlen(val) + 1) + 2; + if ((tst_kvercmp(5, 17, 1)) >= 0) { + for (size_t i = 0; i < 5000; i++) { + /* use same logic in kernel legacy_parse_param function */ + const size_t len = i * (strlen(val) + 2) + (strlen(val) + 1) + 2; - if (!strcmp(tst_device->fs_type, "btrfs") && len <= (size_t)pagesize) - TST_EXP_PASS_SILENT(fsconfig(fd, FSCONFIG_SET_STRING, "\x00", val, 0)); - else - TST_EXP_FAIL_SILENT(fsconfig(fd, FSCONFIG_SET_STRING, "\x00", val, 0), - EINVAL); + if (!strcmp(tst_device->fs_type, "btrfs") && len <= (size_t)pagesize) + TST_EXP_PASS_SILENT(fsconfig(fd, FSCONFIG_SET_STRING, "\x00", val, 0)); + else + TST_EXP_FAIL_SILENT(fsconfig(fd, FSCONFIG_SET_STRING, "\x00", val, 0), + EINVAL); + } + } else { + for (size_t i = 0; i < 5000; i++) + fsconfig(fd, FSCONFIG_SET_STRING, "\x00", val, 0); } if (fd != -1)
Signed-off-by: Wei Gao <wegao@suse.com> --- .../kernel/syscalls/fsconfig/fsconfig03.c | 21 ++++++++++++------- 1 file changed, 13 insertions(+), 8 deletions(-)