| Message ID | 20240606065506.1686-1-lufei@uniontech.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | acct01: add EFAULT errno check | expand |
Hi Lu, > 1. add EFAULT errno check. > 2. fix make check errors and warnings. Changes LGTM, but could you please separate these changes into it's own commit? FYI it's a good habit to separate changes (easier to review, it would not be clear what is EFAULT related change). We sometimes just mix these changes, but here both changes are quite big + you even touch a different test. > --- ... > testcases/kernel/syscalls/acct/acct01.c | 31 +++++++++++++++++-------- > - TEST(acct(NULL)); > - if (TST_RET == -1 && TST_ERR == ENOSYS) > - tst_brk(TCONF, "acct() system call isn't configured in kernel"); Good point. This was added in 2013 ba3bf0e34 ("acct01: add a check routine for acct implementation") and was valid until you added now: > + .needs_kconfigs = (const char *[]) { > + "CONFIG_BSD_PROCESS_ACCT=y", > + }, @Cyril: would you replace ENOSYS with CONFIG_BSD_PROCESS_ACCT=y ? I would try to avoid using .needs_kconfigs when ENOSYS can be checked. Kind regards, Petr > }; > diff --git a/testcases/kernel/syscalls/acct/acct02.c b/testcases/kernel/syscalls/acct/acct02.c > index d3f3d9d04..74019f430 100644 > --- a/testcases/kernel/syscalls/acct/acct02.c > +++ b/testcases/kernel/syscalls/acct/acct02.c > @@ -186,7 +186,7 @@ static void run(void) > if (read_bytes != acct_size) { > tst_res(TFAIL, "incomplete read %i bytes, expected %i", > - read_bytes, acct_size); > + read_bytes, acct_size); > goto exit; > }
Hi! > 1. add EFAULT errno check. > 2. fix make check errors and warnings. Can you please split the functional changes and make chek fixes into separate tests? Ideally each type of change should be put into a separate patch. > Signed-off-by: lufei <lufei@uniontech.com> > --- > testcases/kernel/syscalls/acct/acct01.c | 31 +++++++++++++++++-------- > testcases/kernel/syscalls/acct/acct02.c | 2 +- > 2 files changed, 22 insertions(+), 11 deletions(-) > > diff --git a/testcases/kernel/syscalls/acct/acct01.c b/testcases/kernel/syscalls/acct/acct01.c > index a05ed2ea9..ed1817bc5 100644 > --- a/testcases/kernel/syscalls/acct/acct01.c > +++ b/testcases/kernel/syscalls/acct/acct01.c > @@ -25,8 +25,7 @@ > > #include "tst_test.h" > > -#define DIR_MODE (S_IRUSR|S_IWUSR|S_IXUSR|S_IRGRP| \ > - S_IXGRP|S_IROTH|S_IXOTH) > +#define DIR_MODE 0755 > #define FILE_EISDIR "." > #define FILE_EACCESS "/dev/null" > #define FILE_ENOENT "/tmp/does/not/exist" > @@ -34,6 +33,7 @@ > #define FILE_TMPFILE "./tmpfile" > #define FILE_ELOOP "test_file_eloop1" > #define FILE_EROFS "ro_mntpoint/file" > +#define FILE_EFAULT "/tmp/invalid/file/name" > > static struct passwd *ltpuser; > > @@ -46,6 +46,7 @@ static char *file_eloop; > static char *file_enametoolong; > static char *file_erofs; > static char *file_null; > +static char *file_efault; > > static void setup_euid(void) > { > @@ -57,12 +58,22 @@ static void cleanup_euid(void) > SAFE_SETEUID(0); > } > > +static void setup_emem(void) > +{ > + file_efault = SAFE_MMAP(NULL, 1, PROT_NONE, > + MAP_ANONYMOUS | MAP_PRIVATE, 0, 0); > +} > +static void cleanup_emem(void) > +{ > + SAFE_MUNMAP(file_efault, 1); > +} > + > static struct test_case { > char **filename; > char *desc; > int exp_errno; > - void (*setupfunc) (); > - void (*cleanfunc) (); > + void (*setupfunc)(); > + void (*cleanfunc)(); > } tcases[] = { > {&file_eisdir, FILE_EISDIR, EISDIR, NULL, NULL}, > {&file_eaccess, FILE_EACCESS, EACCES, NULL, NULL}, > @@ -73,16 +84,13 @@ static struct test_case { > {&file_eloop, FILE_ELOOP, ELOOP, NULL, NULL}, > {&file_enametoolong, "aaaa...", ENAMETOOLONG, NULL, NULL}, > {&file_erofs, FILE_EROFS, EROFS, NULL, NULL}, > + {&file_efault, FILE_EFAULT, EFAULT, setup_emem, cleanup_emem}, ^ This should actually describe the testcase so it should be something as "Invalid address" > }; > > static void setup(void) > { > int fd; > > - TEST(acct(NULL)); > - if (TST_RET == -1 && TST_ERR == ENOSYS) > - tst_brk(TCONF, "acct() system call isn't configured in kernel"); > - > ltpuser = SAFE_GETPWNAM("nobody"); > > fd = SAFE_CREAT(FILE_TMPFILE, 0777); > @@ -113,7 +121,7 @@ static void verify_acct(unsigned int nr) > tcase->setupfunc(); > > TST_EXP_FAIL(acct(*tcase->filename), tcase->exp_errno, > - "acct(%s)", tcase->desc); > + "acct(%s)", tcase->desc); > > if (tcase->cleanfunc) > tcase->cleanfunc(); > @@ -136,5 +144,8 @@ static struct tst_test test = { > {&file_enametoolong, .size = PATH_MAX+2}, > {&file_erofs, .str = FILE_EROFS}, > {} > - } > + }, > + .needs_kconfigs = (const char *[]) { > + "CONFIG_BSD_PROCESS_ACCT=y", > + }, > }; And this is a different change that is not described in the patch description. So this patch should be actually split into three patches, one that adds errno check, one that adds needs_kconfigs and one that fixes make check errors. > diff --git a/testcases/kernel/syscalls/acct/acct02.c b/testcases/kernel/syscalls/acct/acct02.c > index d3f3d9d04..74019f430 100644 > --- a/testcases/kernel/syscalls/acct/acct02.c > +++ b/testcases/kernel/syscalls/acct/acct02.c > @@ -186,7 +186,7 @@ static void run(void) > > if (read_bytes != acct_size) { > tst_res(TFAIL, "incomplete read %i bytes, expected %i", > - read_bytes, acct_size); > + read_bytes, acct_size); > goto exit; > } > > -- > 2.39.3 > > > -- > Mailing list info: https://lists.linux.it/listinfo/ltp
Hi Cyril
Thanks for reply.
In the beginning, replace ENOSYS with .needs_kconfigs is for fixing one of the `make check` warnings. Should I still split .needs_kconfigs and make check fixes to seperate patches?
Best reguards.
Lufei
------------------ Original ------------------
From: "Cyril Hrubis"<chrubis@suse.cz>;
Date: Thu, Jul 4, 2024 01:29 PM
To: "lufei"<lufei@uniontech.com>;
Cc: "ltp"<ltp@lists.linux.it>;
Subject: Re: [LTP] [PATCH] acct01: add EFAULT errno check
Hi!
> 1. add EFAULT errno check.
> 2. fix make check errors and warnings.
Can you please split the functional changes and make chek fixes into
separate tests?
Ideally each type of change should be put into a separate patch.
> Signed-off-by: lufei <lufei@uniontech.com>
> ---
> testcases/kernel/syscalls/acct/acct01.c | 31 +++++++++++++++++--------
> testcases/kernel/syscalls/acct/acct02.c | 2 +-
> 2 files changed, 22 insertions(+), 11 deletions(-)
>
> diff --git a/testcases/kernel/syscalls/acct/acct01.c b/testcases/kernel/syscalls/acct/acct01.c
> index a05ed2ea9..ed1817bc5 100644
> --- a/testcases/kernel/syscalls/acct/acct01.c
> +++ b/testcases/kernel/syscalls/acct/acct01.c
> @@ -25,8 +25,7 @@
>
> #include "tst_test.h"
>
> -#define DIR_MODE (S_IRUSR|S_IWUSR|S_IXUSR|S_IRGRP| \
> - S_IXGRP|S_IROTH|S_IXOTH)
> +#define DIR_MODE 0755
> #define FILE_EISDIR "."
> #define FILE_EACCESS "/dev/null"
> #define FILE_ENOENT "/tmp/does/not/exist"
> @@ -34,6 +33,7 @@
> #define FILE_TMPFILE "./tmpfile"
> #define FILE_ELOOP "test_file_eloop1"
> #define FILE_EROFS "ro_mntpoint/file"
> +#define FILE_EFAULT "/tmp/invalid/file/name"
>
> static struct passwd *ltpuser;
>
> @@ -46,6 +46,7 @@ static char *file_eloop;
> static char *file_enametoolong;
> static char *file_erofs;
> static char *file_null;
> +static char *file_efault;
>
> static void setup_euid(void)
> {
> @@ -57,12 +58,22 @@ static void cleanup_euid(void)
> SAFE_SETEUID(0);
> }
>
> +static void setup_emem(void)
> +{
> + file_efault = SAFE_MMAP(NULL, 1, PROT_NONE,
> + MAP_ANONYMOUS | MAP_PRIVATE, 0, 0);
> +}
> +static void cleanup_emem(void)
> +{
> + SAFE_MUNMAP(file_efault, 1);
> +}
> +
> static struct test_case {
> char **filename;
> char *desc;
> int exp_errno;
> - void (*setupfunc) ();
> - void (*cleanfunc) ();
> + void (*setupfunc)();
> + void (*cleanfunc)();
> } tcases[] = {
> {&file_eisdir, FILE_EISDIR, EISDIR, NULL, NULL},
> {&file_eaccess, FILE_EACCESS, EACCES, NULL, NULL},
> @@ -73,16 +84,13 @@ static struct test_case {
> {&file_eloop, FILE_ELOOP, ELOOP, NULL, NULL},
> {&file_enametoolong, "aaaa...", ENAMETOOLONG, NULL, NULL},
> {&file_erofs, FILE_EROFS, EROFS, NULL, NULL},
> + {&file_efault, FILE_EFAULT, EFAULT, setup_emem, cleanup_emem},
^
This should actually describe the testcase so
it should be something as "Invalid address"
> };
>
> static void setup(void)
> {
> int fd;
>
> - TEST(acct(NULL));
> - if (TST_RET == -1 && TST_ERR == ENOSYS)
> - tst_brk(TCONF, "acct() system call isn't configured in kernel");
> -
> ltpuser = SAFE_GETPWNAM("nobody");
>
> fd = SAFE_CREAT(FILE_TMPFILE, 0777);
> @@ -113,7 +121,7 @@ static void verify_acct(unsigned int nr)
> tcase->setupfunc();
>
> TST_EXP_FAIL(acct(*tcase->filename), tcase->exp_errno,
> - "acct(%s)", tcase->desc);
> + "acct(%s)", tcase->desc);
>
> if (tcase->cleanfunc)
> tcase->cleanfunc();
> @@ -136,5 +144,8 @@ static struct tst_test test = {
> {&file_enametoolong, .size = PATH_MAX+2},
> {&file_erofs, .str = FILE_EROFS},
> {}
> - }
> + },
> + .needs_kconfigs = (const char *[]) {
> + "CONFIG_BSD_PROCESS_ACCT=y",
> + },
> };
And this is a different change that is not described in the patch
description. So this patch should be actually split into three patches,
one that adds errno check, one that adds needs_kconfigs and one that
fixes make check errors.
> diff --git a/testcases/kernel/syscalls/acct/acct02.c b/testcases/kernel/syscalls/acct/acct02.c
> index d3f3d9d04..74019f430 100644
> --- a/testcases/kernel/syscalls/acct/acct02.c
> +++ b/testcases/kernel/syscalls/acct/acct02.c
> @@ -186,7 +186,7 @@ static void run(void)
>
> if (read_bytes != acct_size) {
> tst_res(TFAIL, "incomplete read %i bytes, expected %i",
> - read_bytes, acct_size);
> + read_bytes, acct_size);
> goto exit;
> }
>
> --
> 2.39.3
>
>
> --
> Mailing list info: https://lists.linux.it/listinfo/ltp
Hi! > In the beginning, replace ENOSYS with .needs_kconfigs is for fixing > one of the `make check` warnings. Should I still split .needs_kconfigs > and make check fixes to seperate patches? Yes please. Ideally keep one type of a change in each patch.
diff --git a/testcases/kernel/syscalls/acct/acct01.c b/testcases/kernel/syscalls/acct/acct01.c index a05ed2ea9..ed1817bc5 100644 --- a/testcases/kernel/syscalls/acct/acct01.c +++ b/testcases/kernel/syscalls/acct/acct01.c @@ -25,8 +25,7 @@ #include "tst_test.h" -#define DIR_MODE (S_IRUSR|S_IWUSR|S_IXUSR|S_IRGRP| \ - S_IXGRP|S_IROTH|S_IXOTH) +#define DIR_MODE 0755 #define FILE_EISDIR "." #define FILE_EACCESS "/dev/null" #define FILE_ENOENT "/tmp/does/not/exist" @@ -34,6 +33,7 @@ #define FILE_TMPFILE "./tmpfile" #define FILE_ELOOP "test_file_eloop1" #define FILE_EROFS "ro_mntpoint/file" +#define FILE_EFAULT "/tmp/invalid/file/name" static struct passwd *ltpuser; @@ -46,6 +46,7 @@ static char *file_eloop; static char *file_enametoolong; static char *file_erofs; static char *file_null; +static char *file_efault; static void setup_euid(void) { @@ -57,12 +58,22 @@ static void cleanup_euid(void) SAFE_SETEUID(0); } +static void setup_emem(void) +{ + file_efault = SAFE_MMAP(NULL, 1, PROT_NONE, + MAP_ANONYMOUS | MAP_PRIVATE, 0, 0); +} +static void cleanup_emem(void) +{ + SAFE_MUNMAP(file_efault, 1); +} + static struct test_case { char **filename; char *desc; int exp_errno; - void (*setupfunc) (); - void (*cleanfunc) (); + void (*setupfunc)(); + void (*cleanfunc)(); } tcases[] = { {&file_eisdir, FILE_EISDIR, EISDIR, NULL, NULL}, {&file_eaccess, FILE_EACCESS, EACCES, NULL, NULL}, @@ -73,16 +84,13 @@ static struct test_case { {&file_eloop, FILE_ELOOP, ELOOP, NULL, NULL}, {&file_enametoolong, "aaaa...", ENAMETOOLONG, NULL, NULL}, {&file_erofs, FILE_EROFS, EROFS, NULL, NULL}, + {&file_efault, FILE_EFAULT, EFAULT, setup_emem, cleanup_emem}, }; static void setup(void) { int fd; - TEST(acct(NULL)); - if (TST_RET == -1 && TST_ERR == ENOSYS) - tst_brk(TCONF, "acct() system call isn't configured in kernel"); - ltpuser = SAFE_GETPWNAM("nobody"); fd = SAFE_CREAT(FILE_TMPFILE, 0777); @@ -113,7 +121,7 @@ static void verify_acct(unsigned int nr) tcase->setupfunc(); TST_EXP_FAIL(acct(*tcase->filename), tcase->exp_errno, - "acct(%s)", tcase->desc); + "acct(%s)", tcase->desc); if (tcase->cleanfunc) tcase->cleanfunc(); @@ -136,5 +144,8 @@ static struct tst_test test = { {&file_enametoolong, .size = PATH_MAX+2}, {&file_erofs, .str = FILE_EROFS}, {} - } + }, + .needs_kconfigs = (const char *[]) { + "CONFIG_BSD_PROCESS_ACCT=y", + }, }; diff --git a/testcases/kernel/syscalls/acct/acct02.c b/testcases/kernel/syscalls/acct/acct02.c index d3f3d9d04..74019f430 100644 --- a/testcases/kernel/syscalls/acct/acct02.c +++ b/testcases/kernel/syscalls/acct/acct02.c @@ -186,7 +186,7 @@ static void run(void) if (read_bytes != acct_size) { tst_res(TFAIL, "incomplete read %i bytes, expected %i", - read_bytes, acct_size); + read_bytes, acct_size); goto exit; }
1. add EFAULT errno check. 2. fix make check errors and warnings. Signed-off-by: lufei <lufei@uniontech.com> --- testcases/kernel/syscalls/acct/acct01.c | 31 +++++++++++++++++-------- testcases/kernel/syscalls/acct/acct02.c | 2 +- 2 files changed, 22 insertions(+), 11 deletions(-)