[LTP] [PATCH] containers/utsname04: don't use TST_EXP_EQ_LI for tst_clone

[LTP] [PATCH] containers/utsname04: don't use TST_EXP_EQ_LI for tst_clone

[Yang Xu]

For old kernel that doesn't support clone3, tst_clone returns
-2 instead of -1. So we can't use TST_EXP_EQ_LI api.

Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
 testcases/kernel/containers/utsname/utsname04.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/testcases/kernel/containers/utsname/utsname04.c b/testcases/kernel/containers/utsname/utsname04.c
index e8d636d0d..1adc4542c 100644
--- a/testcases/kernel/containers/utsname/utsname04.c
+++ b/testcases/kernel/containers/utsname/utsname04.c
@@ -22,6 +22,7 @@ static void run(void)
 {
 	const struct tst_clone_args cargs = { CLONE_NEWUTS, SIGCHLD };
 	struct passwd *pw;
+	pid_t pid;
 
 	tst_res(TINFO, "Dropping root privileges");
 
@@ -29,7 +30,11 @@ static void run(void)
 	SAFE_SETRESUID(pw->pw_uid, pw->pw_uid, pw->pw_uid);
 
 	if (!str_op || !strcmp(str_op, "clone")) {
-		TST_EXP_EQ_LI(tst_clone(&cargs), -1);
+		pid = tst_clone(&cargs);
+		if (pid == -1 || pid == -2)
+			tst_res(TPASS, "clone() returns an expected value %d", pid);
+		else
+			tst_res(TFAIL, "clone() returns an unexpected value %d", pid);
 		TST_EXP_PASS(errno == EPERM);
 	} else {
 		if (!SAFE_FORK()) {
-- 
2.39.1


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

Re: [LTP] [PATCH] containers/utsname04: don't use TST_EXP_EQ_LI for tst_clone

[Andrea Cervesato via ltp]

Hi!

On 2/28/23 09:44, Yang Xu wrote:

> For old kernel that doesn't support clone3, tst_clone returns
it would be nice to add unsupported kernel version inside patch comment 
(in this case, anything below 5.3).
> -2 instead of -1. So we can't use TST_EXP_EQ_LI api.
>
> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
> ---
>   testcases/kernel/containers/utsname/utsname04.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/testcases/kernel/containers/utsname/utsname04.c b/testcases/kernel/containers/utsname/utsname04.c
> index e8d636d0d..1adc4542c 100644
> --- a/testcases/kernel/containers/utsname/utsname04.c
> +++ b/testcases/kernel/containers/utsname/utsname04.c
> @@ -22,6 +22,7 @@ static void run(void)
>   {
>   	const struct tst_clone_args cargs = { CLONE_NEWUTS, SIGCHLD };
>   	struct passwd *pw;
> +	pid_t pid;
>   
>   	tst_res(TINFO, "Dropping root privileges");
>   
> @@ -29,7 +30,11 @@ static void run(void)
>   	SAFE_SETRESUID(pw->pw_uid, pw->pw_uid, pw->pw_uid);
>   
>   	if (!str_op || !strcmp(str_op, "clone")) {
> -		TST_EXP_EQ_LI(tst_clone(&cargs), -1);

Did you try this?

TST_EXP_FAIL(tst_clone(&cargs), EPERM);

> +		pid = tst_clone(&cargs);
> +		if (pid == -1 || pid == -2)
> +			tst_res(TPASS, "clone() returns an expected value %d", pid);
> +		else
> +			tst_res(TFAIL, "clone() returns an unexpected value %d", pid);
>   		TST_EXP_PASS(errno == EPERM);
>   	} else {
>   		if (!SAFE_FORK()) {

Regards,
Andrea


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

Re: [LTP] [PATCH] containers/utsname04: don't use TST_EXP_EQ_LI for tst_clone

[xuyang2018.jy]

Hi Andrea

> Hi!
> 
> On 2/28/23 09:44, Yang Xu wrote:
> 
>> For old kernel that doesn't support clone3, tst_clone returns
> it would be nice to add unsupported kernel version inside patch comment 
> (in this case, anything below 5.3).

Yes.
>> -2 instead of -1. So we can't use TST_EXP_EQ_LI api.
>>
>> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
>> ---
>>   testcases/kernel/containers/utsname/utsname04.c | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/testcases/kernel/containers/utsname/utsname04.c 
>> b/testcases/kernel/containers/utsname/utsname04.c
>> index e8d636d0d..1adc4542c 100644
>> --- a/testcases/kernel/containers/utsname/utsname04.c
>> +++ b/testcases/kernel/containers/utsname/utsname04.c
>> @@ -22,6 +22,7 @@ static void run(void)
>>   {
>>       const struct tst_clone_args cargs = { CLONE_NEWUTS, SIGCHLD };
>>       struct passwd *pw;
>> +    pid_t pid;
>>       tst_res(TINFO, "Dropping root privileges");
>> @@ -29,7 +30,11 @@ static void run(void)
>>       SAFE_SETRESUID(pw->pw_uid, pw->pw_uid, pw->pw_uid);
>>       if (!str_op || !strcmp(str_op, "clone")) {
>> -        TST_EXP_EQ_LI(tst_clone(&cargs), -1);
> 
> Did you try this?
> 
> TST_EXP_FAIL(tst_clone(&cargs), EPERM);

I don't try this because I remembered TST_EXP_FAIL only thinks the 
correct return value is -1.

tst_test.c:1560: TINFO: Timeout per run is 0h 00m 30s
utsname04.c:27: TINFO: Dropping root privileges
utsname04.c:33: TFAIL: tst_clone(&cargs) invalid retval -2: EPERM (1)

Best Regards
Yang Xu
> 
>> +        pid = tst_clone(&cargs);
>> +        if (pid == -1 || pid == -2)
>> +            tst_res(TPASS, "clone() returns an expected value %d", pid);
>> +        else
>> +            tst_res(TFAIL, "clone() returns an unexpected value %d", 
>> pid);
>>           TST_EXP_PASS(errno == EPERM);
>>       } else {
>>           if (!SAFE_FORK()) {
> 
> Regards,
> Andrea
> 
> 

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

Re: [LTP] [PATCH] containers/utsname04: don't use TST_EXP_EQ_LI for tst_clone

[Cyril Hrubis]

Hi!
> > Did you try this?
> > 
> > TST_EXP_FAIL(tst_clone(&cargs), EPERM);
> 
> I don't try this because I remembered TST_EXP_FAIL only thinks the 
> correct return value is -1.
> 
> tst_test.c:1560: TINFO: Timeout per run is 0h 00m 30s
> utsname04.c:27: TINFO: Dropping root privileges
> utsname04.c:33: TFAIL: tst_clone(&cargs) invalid retval -2: EPERM (1)

It's actually the tst_clone() that returns -2 if fallback to __NR_clone
failed.

@Ritchie Is there actually a good reason why tst_clone() returns -2 on a
         fialure? Can we fix the code by:

diff --git a/lib/tst_clone.c b/lib/tst_clone.c
index ecc84408c..bacd269d9 100644
--- a/lib/tst_clone.c
+++ b/lib/tst_clone.c
@@ -39,8 +39,5 @@ pid_t tst_clone(const struct tst_clone_args *tst_args)
        pid = syscall(__NR_clone, flags, NULL);
 #endif
 
-       if (pid == -1)
-               return -2;
-
        return pid;
 }


-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH] containers/utsname04: don't use TST_EXP_EQ_LI for tst_clone

[xuyang2018.jy]

Hi Cyril

> Hi!
>>> Did you try this?
>>>
>>> TST_EXP_FAIL(tst_clone(&cargs), EPERM);
>>
>> I don't try this because I remembered TST_EXP_FAIL only thinks the
>> correct return value is -1.
>>
>> tst_test.c:1560: TINFO: Timeout per run is 0h 00m 30s
>> utsname04.c:27: TINFO: Dropping root privileges
>> utsname04.c:33: TFAIL: tst_clone(&cargs) invalid retval -2: EPERM (1)
> 
> It's actually the tst_clone() that returns -2 if fallback to __NR_clone
> failed.
> 
> @Ritchie Is there actually a good reason why tst_clone() returns -2 on a
>           fialure? Can we fix the code by:

I guess it is used to distinguish clone3(-1) and clone failure(-2).
@Ritchie  Is this right? Or you have other meaning.

Maybe we can use clone instead of tst_clone in this case?

Best Regards
Yang Xu
  >
> diff --git a/lib/tst_clone.c b/lib/tst_clone.c
> index ecc84408c..bacd269d9 100644
> --- a/lib/tst_clone.c
> +++ b/lib/tst_clone.c
> @@ -39,8 +39,5 @@ pid_t tst_clone(const struct tst_clone_args *tst_args)
>          pid = syscall(__NR_clone, flags, NULL);
>   #endif
>   
> -       if (pid == -1)
> -               return -2;
> -
>          return pid;
>   }
> 
> 

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

Re: [LTP] [PATCH] containers/utsname04: don't use TST_EXP_EQ_LI for tst_clone

[Richard Palethorpe]

Hello,

"xuyang2018.jy@fujitsu.com" <xuyang2018.jy@fujitsu.com> writes:

> Hi Cyril
>
>> Hi!
>>>> Did you try this?
>>>>
>>>> TST_EXP_FAIL(tst_clone(&cargs), EPERM);
>>>
>>> I don't try this because I remembered TST_EXP_FAIL only thinks the
>>> correct return value is -1.
>>>
>>> tst_test.c:1560: TINFO: Timeout per run is 0h 00m 30s
>>> utsname04.c:27: TINFO: Dropping root privileges
>>> utsname04.c:33: TFAIL: tst_clone(&cargs) invalid retval -2: EPERM (1)
>> 
>> It's actually the tst_clone() that returns -2 if fallback to __NR_clone
>> failed.
>> 
>> @Ritchie Is there actually a good reason why tst_clone() returns -2 on a
>>           fialure? Can we fix the code by:
>
> I guess it is used to distinguish clone3(-1) and clone failure(-2).
> @Ritchie  Is this right? Or you have other meaning.

Yes, this is how it is used in SAFE_CLONE to provide debug information
when tst_clone fails.

It's important to know which clone failed as obviously they do not
support all the same flags or features.

>
> Maybe we can use clone instead of tst_clone in this case?

I think just use the return value as it was intended and print which
version of clone failed.

>
> Best Regards
> Yang Xu
>   >
>> diff --git a/lib/tst_clone.c b/lib/tst_clone.c
>> index ecc84408c..bacd269d9 100644
>> --- a/lib/tst_clone.c
>> +++ b/lib/tst_clone.c
>> @@ -39,8 +39,5 @@ pid_t tst_clone(const struct tst_clone_args *tst_args)
>>          pid = syscall(__NR_clone, flags, NULL);
>>   #endif
>>   
>> -       if (pid == -1)
>> -               return -2;
>> -
>>          return pid;
>>   }
>> 
>> 


-- 
Thank you,
Richard.

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

[LTP] [PATCH v2] containers/utsname04: don't use TST_EXP_EQ_LI for tst_clone

[Yang Xu]

For old kernel(below 5.3) that doesn't support clone3, tst_clone returns
-2 instead of -1. So we can't use TST_EXP_EQ_LI api.

Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
 testcases/kernel/containers/utsname/utsname04.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/testcases/kernel/containers/utsname/utsname04.c b/testcases/kernel/containers/utsname/utsname04.c
index e8d636d0d..61cc066d8 100644
--- a/testcases/kernel/containers/utsname/utsname04.c
+++ b/testcases/kernel/containers/utsname/utsname04.c
@@ -29,7 +29,13 @@ static void run(void)
 	SAFE_SETRESUID(pw->pw_uid, pw->pw_uid, pw->pw_uid);
 
 	if (!str_op || !strcmp(str_op, "clone")) {
-		TST_EXP_EQ_LI(tst_clone(&cargs), -1);
+		TEST(tst_clone(&cargs));
+		if (TST_RET == -1)
+			tst_res(TPASS, "clone3() fails as expected");
+		else if (TST_RET == -2)
+			tst_res(TPASS, "clone() fails as expected");
+		else
+			tst_res(TFAIL, "tst_clone returns %ld", TST_RET);
 		TST_EXP_PASS(errno == EPERM);
 	} else {
 		if (!SAFE_FORK()) {
-- 
2.39.1


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

Re: [LTP] [PATCH v2] containers/utsname04: don't use TST_EXP_EQ_LI for tst_clone

[Richard Palethorpe]

Hello,

Yang Xu <xuyang2018.jy@fujitsu.com> writes:

> For old kernel(below 5.3) that doesn't support clone3, tst_clone returns
> -2 instead of -1. So we can't use TST_EXP_EQ_LI api.
>
> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>

Reviewed-by: Richard Palethorpe <rpalethorpe@suse.com>

Thanks, I suppose this could be avoided in the future by hinting that
the return type is more complex than the usual for clone

typedef tst_clone_return_t pid_t;

enum tst_clone_error {
  TST_CLONE_ERROR = -2,
  TST_CLONE3_ERROR = -1
};

Possibly we could force a cast somehow? Otherwise it's just a hint.

> ---
>  testcases/kernel/containers/utsname/utsname04.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/testcases/kernel/containers/utsname/utsname04.c b/testcases/kernel/containers/utsname/utsname04.c
> index e8d636d0d..61cc066d8 100644
> --- a/testcases/kernel/containers/utsname/utsname04.c
> +++ b/testcases/kernel/containers/utsname/utsname04.c
> @@ -29,7 +29,13 @@ static void run(void)
>  	SAFE_SETRESUID(pw->pw_uid, pw->pw_uid, pw->pw_uid);
>  
>  	if (!str_op || !strcmp(str_op, "clone")) {
> -		TST_EXP_EQ_LI(tst_clone(&cargs), -1);
> +		TEST(tst_clone(&cargs));
> +		if (TST_RET == -1)
> +			tst_res(TPASS, "clone3() fails as expected");
> +		else if (TST_RET == -2)
> +			tst_res(TPASS, "clone() fails as expected");
> +		else
> +			tst_res(TFAIL, "tst_clone returns %ld", TST_RET);
>  		TST_EXP_PASS(errno == EPERM);
>  	} else {
>  		if (!SAFE_FORK()) {
> -- 
> 2.39.1


-- 
Thank you,
Richard.

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

Re: [LTP] [PATCH v2] containers/utsname04: don't use TST_EXP_EQ_LI for tst_clone

[Richard Palethorpe]

Hello,

Merged, thanks!

Richard Palethorpe <rpalethorpe@suse.de> writes:

> Hello,
>
> Yang Xu <xuyang2018.jy@fujitsu.com> writes:
>
>> For old kernel(below 5.3) that doesn't support clone3, tst_clone returns
>> -2 instead of -1. So we can't use TST_EXP_EQ_LI api.
>>
>> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
>
> Reviewed-by: Richard Palethorpe <rpalethorpe@suse.com>
>
> Thanks, I suppose this could be avoided in the future by hinting that
> the return type is more complex than the usual for clone
>
> typedef tst_clone_return_t pid_t;
>
> enum tst_clone_error {
>   TST_CLONE_ERROR = -2,
>   TST_CLONE3_ERROR = -1
> };
>
> Possibly we could force a cast somehow? Otherwise it's just a hint.
>
>> ---
>>  testcases/kernel/containers/utsname/utsname04.c | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/testcases/kernel/containers/utsname/utsname04.c b/testcases/kernel/containers/utsname/utsname04.c
>> index e8d636d0d..61cc066d8 100644
>> --- a/testcases/kernel/containers/utsname/utsname04.c
>> +++ b/testcases/kernel/containers/utsname/utsname04.c
>> @@ -29,7 +29,13 @@ static void run(void)
>>  	SAFE_SETRESUID(pw->pw_uid, pw->pw_uid, pw->pw_uid);
>>  
>>  	if (!str_op || !strcmp(str_op, "clone")) {
>> -		TST_EXP_EQ_LI(tst_clone(&cargs), -1);
>> +		TEST(tst_clone(&cargs));
>> +		if (TST_RET == -1)
>> +			tst_res(TPASS, "clone3() fails as expected");
>> +		else if (TST_RET == -2)
>> +			tst_res(TPASS, "clone() fails as expected");
>> +		else
>> +			tst_res(TFAIL, "tst_clone returns %ld", TST_RET);
>>  		TST_EXP_PASS(errno == EPERM);
>>  	} else {
>>  		if (!SAFE_FORK()) {
>> -- 
>> 2.39.1
>
>
> -- 
> Thank you,
> Richard.


-- 
Thank you,
Richard.

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