Re: [LTP] [PATCH] acct01: add EFAULT errno check

[路斐 <lufei@uniontech.com>]

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: &nbsp;"Cyril&nbsp;Hrubis"<chrubis@suse.cz&gt;;
Date: &nbsp;Thu, Jul 4, 2024 01:29 PM
To: &nbsp;"lufei"<lufei@uniontech.com&gt;; 
Cc: &nbsp;"ltp"<ltp@lists.linux.it&gt;; 
Subject: &nbsp;Re: [LTP] [PATCH] acct01: add EFAULT errno check

&nbsp;

Hi!
&gt; 1. add EFAULT errno check.
&gt; 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.

&gt; Signed-off-by: lufei <lufei@uniontech.com&gt;
&gt; ---
&gt;&nbsp; testcases/kernel/syscalls/acct/acct01.c | 31 +++++++++++++++++--------
&gt;&nbsp; testcases/kernel/syscalls/acct/acct02.c |&nbsp; 2 +-
&gt;&nbsp; 2 files changed, 22 insertions(+), 11 deletions(-)
&gt; 
&gt; diff --git a/testcases/kernel/syscalls/acct/acct01.c b/testcases/kernel/syscalls/acct/acct01.c
&gt; index a05ed2ea9..ed1817bc5 100644
&gt; --- a/testcases/kernel/syscalls/acct/acct01.c
&gt; +++ b/testcases/kernel/syscalls/acct/acct01.c
&gt; @@ -25,8 +25,7 @@
&gt;&nbsp; 
&gt;&nbsp; #include "tst_test.h"
&gt;&nbsp; 
&gt; -#define DIR_MODE	(S_IRUSR|S_IWUSR|S_IXUSR|S_IRGRP| \
&gt; -			 S_IXGRP|S_IROTH|S_IXOTH)
&gt; +#define DIR_MODE	0755
&gt;&nbsp; #define FILE_EISDIR		"."
&gt;&nbsp; #define FILE_EACCESS		"/dev/null"
&gt;&nbsp; #define FILE_ENOENT		"/tmp/does/not/exist"
&gt; @@ -34,6 +33,7 @@
&gt;&nbsp; #define FILE_TMPFILE		"./tmpfile"
&gt;&nbsp; #define FILE_ELOOP		"test_file_eloop1"
&gt;&nbsp; #define FILE_EROFS		"ro_mntpoint/file"
&gt; +#define FILE_EFAULT		"/tmp/invalid/file/name"
&gt;&nbsp; 
&gt;&nbsp; static struct passwd *ltpuser;
&gt;&nbsp; 
&gt; @@ -46,6 +46,7 @@ static char *file_eloop;
&gt;&nbsp; static char *file_enametoolong;
&gt;&nbsp; static char *file_erofs;
&gt;&nbsp; static char *file_null;
&gt; +static char *file_efault;
&gt;&nbsp; 
&gt;&nbsp; static void setup_euid(void)
&gt;&nbsp; {
&gt; @@ -57,12 +58,22 @@ static void cleanup_euid(void)
&gt;&nbsp; 	SAFE_SETEUID(0);
&gt;&nbsp; }
&gt;&nbsp; 
&gt; +static void setup_emem(void)
&gt; +{
&gt; +	file_efault = SAFE_MMAP(NULL, 1, PROT_NONE,
&gt; +			MAP_ANONYMOUS | MAP_PRIVATE, 0, 0);
&gt; +}
&gt; +static void cleanup_emem(void)
&gt; +{
&gt; +	SAFE_MUNMAP(file_efault, 1);
&gt; +}
&gt; +
&gt;&nbsp; static struct test_case {
&gt;&nbsp; 	char **filename;
&gt;&nbsp; 	char *desc;
&gt;&nbsp; 	int exp_errno;
&gt; -	void (*setupfunc) ();
&gt; -	void (*cleanfunc) ();
&gt; +	void (*setupfunc)();
&gt; +	void (*cleanfunc)();
&gt;&nbsp; } tcases[] = {
&gt;&nbsp; 	{&amp;file_eisdir,&nbsp; FILE_EISDIR,&nbsp; EISDIR,&nbsp; NULL,&nbsp;&nbsp; NULL},
&gt;&nbsp; 	{&amp;file_eaccess, FILE_EACCESS, EACCES,&nbsp; NULL,&nbsp;&nbsp; NULL},
&gt; @@ -73,16 +84,13 @@ static struct test_case {
&gt;&nbsp; 	{&amp;file_eloop,&nbsp;&nbsp; FILE_ELOOP,&nbsp;&nbsp; ELOOP,&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; NULL, NULL},
&gt;&nbsp; 	{&amp;file_enametoolong, "aaaa...", ENAMETOOLONG, NULL, NULL},
&gt;&nbsp; 	{&amp;file_erofs,&nbsp;&nbsp; FILE_EROFS,&nbsp;&nbsp; EROFS,&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; NULL, NULL},
&gt; +	{&amp;file_efault,	FILE_EFAULT,&nbsp; EFAULT,&nbsp; setup_emem, cleanup_emem},
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; ^
			 This should actually describe the testcase so
			 it should be something as "Invalid address"

&gt;&nbsp; };
&gt;&nbsp; 
&gt;&nbsp; static void setup(void)
&gt;&nbsp; {
&gt;&nbsp; 	int fd;
&gt;&nbsp; 
&gt; -	TEST(acct(NULL));
&gt; -	if (TST_RET == -1 &amp;&amp; TST_ERR == ENOSYS)
&gt; -		tst_brk(TCONF, "acct() system call isn't configured in kernel");
&gt; -
&gt;&nbsp; 	ltpuser = SAFE_GETPWNAM("nobody");
&gt;&nbsp; 
&gt;&nbsp; 	fd = SAFE_CREAT(FILE_TMPFILE, 0777);
&gt; @@ -113,7 +121,7 @@ static void verify_acct(unsigned int nr)
&gt;&nbsp; 		tcase-&gt;setupfunc();
&gt;&nbsp; 
&gt;&nbsp; 	TST_EXP_FAIL(acct(*tcase-&gt;filename), tcase-&gt;exp_errno,
&gt; -	&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; "acct(%s)", tcase-&gt;desc);
&gt; +			"acct(%s)", tcase-&gt;desc);
&gt;&nbsp; 
&gt;&nbsp; 	if (tcase-&gt;cleanfunc)
&gt;&nbsp; 		tcase-&gt;cleanfunc();
&gt; @@ -136,5 +144,8 @@ static struct tst_test test = {
&gt;&nbsp; 		{&amp;file_enametoolong, .size = PATH_MAX+2},
&gt;&nbsp; 		{&amp;file_erofs, .str = FILE_EROFS},
&gt;&nbsp; 		{}
&gt; -	}
&gt; +	},
&gt; +	.needs_kconfigs = (const char *[]) {
&gt; +		"CONFIG_BSD_PROCESS_ACCT=y",
&gt; +	},
&gt;&nbsp; };

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.

&gt; diff --git a/testcases/kernel/syscalls/acct/acct02.c b/testcases/kernel/syscalls/acct/acct02.c
&gt; index d3f3d9d04..74019f430 100644
&gt; --- a/testcases/kernel/syscalls/acct/acct02.c
&gt; +++ b/testcases/kernel/syscalls/acct/acct02.c
&gt; @@ -186,7 +186,7 @@ static void run(void)
&gt;&nbsp; 
&gt;&nbsp; 		if (read_bytes != acct_size) {
&gt;&nbsp; 			tst_res(TFAIL, "incomplete read %i bytes, expected %i",
&gt; -			&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; read_bytes, acct_size);
&gt; +					read_bytes, acct_size);
&gt;&nbsp; 			goto exit;
&gt;&nbsp; 		}
&gt;&nbsp; 
&gt; -- 
&gt; 2.39.3
&gt; 
&gt; 
&gt; -- 
&gt; Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

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