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

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

[lufei]

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(-)

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;
 		}
 
-- 
2.39.3


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

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

[Petr Vorel]

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;
>  		}

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

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

[lufei]

Add EFAULT errno check in acct01 testcase.

Signed-off-by: lufei <lufei@uniontech.com>
---
 testcases/kernel/syscalls/acct/acct01.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/testcases/kernel/syscalls/acct/acct01.c b/testcases/kernel/syscalls/acct/acct01.c
index 1b53a32f2..ed1817bc5 100644
--- a/testcases/kernel/syscalls/acct/acct01.c
+++ b/testcases/kernel/syscalls/acct/acct01.c
@@ -33,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;
 
@@ -45,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)
 {
@@ -56,6 +58,16 @@ 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;
@@ -72,6 +84,7 @@ 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)
-- 
2.39.3


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

[LTP] [PATCH] acct: fix make check errors: using .needs_kconfigs

[lufei]

fix make check errors and warnings. Using .needs_kconfigs instead of ENOSYS.

Signed-off-by: lufei <lufei@uniontech.com>
---
 testcases/kernel/syscalls/acct/acct01.c | 18 ++++++++----------
 testcases/kernel/syscalls/acct/acct02.c |  2 +-
 2 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/testcases/kernel/syscalls/acct/acct01.c b/testcases/kernel/syscalls/acct/acct01.c
index a05ed2ea9..1b53a32f2 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"
@@ -61,8 +60,8 @@ 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},
@@ -79,10 +78,6 @@ 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 +108,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 +131,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;
 		}
 
-- 
2.39.3


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

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

[Cyril Hrubis]

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

-- 
Cyril Hrubis
chrubis@suse.cz

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

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

[路斐]

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

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

[Petr Vorel]

Hi 
> Add EFAULT errno check in acct01 testcase.

> Signed-off-by: lufei <lufei@uniontech.com>

I guess you don't mind if I change this to following before merge:
Signed-off-by: Lu Fei <lufei@uniontech.com>

> ---
>  testcases/kernel/syscalls/acct/acct01.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)

> diff --git a/testcases/kernel/syscalls/acct/acct01.c b/testcases/kernel/syscalls/acct/acct01.c
> index 1b53a32f2..ed1817bc5 100644
> --- a/testcases/kernel/syscalls/acct/acct01.c
> +++ b/testcases/kernel/syscalls/acct/acct01.c
> @@ -33,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"

And here I just amend to:
#define FILE_EFAULT            "invalid/file/name"

(although it's very unlikely, the full patch *could* be existing, but not the
relative one, because LTP is creating unique temporary directory for each run,
e.g. /tmp/LTP_accTJpYqc.

>  static struct passwd *ltpuser;

> @@ -45,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)
>  {
> @@ -56,6 +58,16 @@ 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;
> @@ -72,6 +84,7 @@ 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},
    {&file_efault,  "Invalid address",  EFAULT,  setup_emem, cleanup_emem},
(as Cyril suggested)

Actually this second version does only single thing (unlike the previous version
[1]), thus I suggest to merge it:
Reviewed-by: Petr Vorel <pvorel@suse.cz>

Therefore I reopen it in patchwork [2].

And you can send warning cleanup after merging this?

Could you, please, next time put version to your patchset (e.g. -v2 for second
version), this help to avoid confusions?

Kind regards,
Petr

[1] https://lore.kernel.org/ltp/20240606065506.1686-1-lufei@uniontech.com/
[2] https://patchwork.ozlabs.org/project/ltp/patch/20240624015245.54968-2-lufei@uniontech.com/

>  };

>  static void setup(void)

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

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

[路斐]

Hi Petr, Cyril.
Really thanks for the advises. Each of them makes me learned.


Sorry for the patches not so good I've sent, I'm trying to make it better in the future.
I will make warnings clearnup patches after this one merged.


Best reguards.
Lu Fei
 
------------------ Original ------------------
From: &nbsp;"Petr&nbsp;Vorel"<pvorel@suse.cz&gt;;
Date: &nbsp;Mon, Jul 8, 2024 04:23 AM
To: &nbsp;"ltp"<ltp@lists.linux.it&gt;; "lufei"<lufei@uniontech.com&gt;; 
Cc: &nbsp;"Cyril Hrubis"<chrubis@suse.cz&gt;; 
Subject: &nbsp;Re: [LTP] [PATCH] acct01: add EFAULT errno check.

&nbsp;

Hi 
&gt; Add EFAULT errno check in acct01 testcase.

&gt; Signed-off-by: lufei <lufei@uniontech.com&gt;

I guess you don't mind if I change this to following before merge:
Signed-off-by: Lu Fei <lufei@uniontech.com&gt;

&gt; ---
&gt;&nbsp; testcases/kernel/syscalls/acct/acct01.c | 13 +++++++++++++
&gt;&nbsp; 1 file changed, 13 insertions(+)

&gt; diff --git a/testcases/kernel/syscalls/acct/acct01.c b/testcases/kernel/syscalls/acct/acct01.c
&gt; index 1b53a32f2..ed1817bc5 100644
&gt; --- a/testcases/kernel/syscalls/acct/acct01.c
&gt; +++ b/testcases/kernel/syscalls/acct/acct01.c
&gt; @@ -33,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"

And here I just amend to:
#define FILE_EFAULT&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; "invalid/file/name"

(although it's very unlikely, the full patch *could* be existing, but not the
relative one, because LTP is creating unique temporary directory for each run,
e.g. /tmp/LTP_accTJpYqc.

&gt;&nbsp; static struct passwd *ltpuser;

&gt; @@ -45,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; static void setup_euid(void)
&gt;&nbsp; {
&gt; @@ -56,6 +58,16 @@ static void cleanup_euid(void)
&gt;&nbsp; 	SAFE_SETEUID(0);
&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; @@ -72,6 +84,7 @@ 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; {&amp;file_efault,&nbsp; "Invalid address",&nbsp; EFAULT,&nbsp; setup_emem, cleanup_emem},
(as Cyril suggested)

Actually this second version does only single thing (unlike the previous version
[1]), thus I suggest to merge it:
Reviewed-by: Petr Vorel <pvorel@suse.cz&gt;

Therefore I reopen it in patchwork [2].

And you can send warning cleanup after merging this?

Could you, please, next time put version to your patchset (e.g. -v2 for second
version), this help to avoid confusions?

Kind regards,
Petr

[1] https://lore.kernel.org/ltp/20240606065506.1686-1-lufei@uniontech.com/
[2] https://patchwork.ozlabs.org/project/ltp/patch/20240624015245.54968-2-lufei@uniontech.com/

&gt;&nbsp; };

&gt;&nbsp; static void setup(void)

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

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

[Petr Vorel]

Hi Lu,

> Hi Petr, Cyril.
> Really thanks for the advises. Each of them makes me learned.

You're welcome. Also looking at XHTML entity '>' and ' ' in your reply below,
it would be great if you could set your email client (MUA) which you use for LTP
mailing list to use plain text (not XHTML).

Here are some tips:
https://www.kernel.org/doc/html/latest/process/email-clients.html

Kind regards,
Petr

> Sorry for the patches not so good I've sent, I'm trying to make it better in the future.
> I will make warnings clearnup patches after this one merged.


> Best reguards.
> Lu Fei
>  
> ------------------ Original ------------------
> From: &nbsp;"Petr&nbsp;Vorel"<pvorel@suse.cz&gt;;
> Date: &nbsp;Mon, Jul 8, 2024 04:23 AM
> To: &nbsp;"ltp"<ltp@lists.linux.it&gt;; "lufei"<lufei@uniontech.com&gt;; 
> Cc: &nbsp;"Cyril Hrubis"<chrubis@suse.cz&gt;; 
> Subject: &nbsp;Re: [LTP] [PATCH] acct01: add EFAULT errno check.

> &nbsp;

> Hi 
> &gt; Add EFAULT errno check in acct01 testcase.

> &gt; Signed-off-by: lufei <lufei@uniontech.com&gt;

> I guess you don't mind if I change this to following before merge:
> Signed-off-by: Lu Fei <lufei@uniontech.com&gt;

> &gt; ---
> &gt;&nbsp; testcases/kernel/syscalls/acct/acct01.c | 13 +++++++++++++
> &gt;&nbsp; 1 file changed, 13 insertions(+)

> &gt; diff --git a/testcases/kernel/syscalls/acct/acct01.c b/testcases/kernel/syscalls/acct/acct01.c
> &gt; index 1b53a32f2..ed1817bc5 100644
> &gt; --- a/testcases/kernel/syscalls/acct/acct01.c
> &gt; +++ b/testcases/kernel/syscalls/acct/acct01.c
> &gt; @@ -33,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"
....

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

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

[Cyril Hrubis]

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.

-- 
Cyril Hrubis
chrubis@suse.cz

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

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

[Petr Vorel]

Hi Lu,

merged, thanks!

Kind regards,
Petr

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