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

[路斐 <lufei@uniontech.com>]

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