[PATCH 1/2] selftests/powerpc: Allocate base registers

[PATCH 1/2] selftests/powerpc: Allocate base registers

[Breno Leitao]

Some ptrace selftests are passing input operands using a constraint that
can allocate any register for the operand, and using these registers on
load/store operations.

If the register allocated by the compiler happens to be zero (r0), it might
cause an invalid memory address access, since load and store operations
consider the content of 0x0 address if the base register is r0, instead
of the content of the r0 register. For example:

	r1 := 0xdeadbeef
	r0 := 0xdeadbeef
	
	ld r2, 0(r1) /* will load into r2 the content of r1 address */
	ld r2, 0(r0) /* will load into r2 the context of 0x0 */

In order to avoid this possible problem, the inline assembly constraint
should be aware that these registers will be used as a base register, thus,
r0 should not be alocated.

Other than that, this patch removes inline assembly operands that are not used
by the tests.

Signed-off-by: Breno Leitao <leitao@debian.org>
---
 tools/testing/selftests/powerpc/ptrace/ptrace-gpr.c        | 2 +-
 tools/testing/selftests/powerpc/ptrace/ptrace-tm-gpr.c     | 4 ++--
 tools/testing/selftests/powerpc/ptrace/ptrace-tm-spd-tar.c | 2 +-
 tools/testing/selftests/powerpc/ptrace/ptrace-tm-spd-vsx.c | 3 +--
 tools/testing/selftests/powerpc/ptrace/ptrace-tm-spr.c     | 2 +-
 tools/testing/selftests/powerpc/ptrace/ptrace-tm-tar.c     | 2 +-
 tools/testing/selftests/powerpc/ptrace/ptrace-tm-vsx.c     | 3 +--
 7 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/tools/testing/selftests/powerpc/ptrace/ptrace-gpr.c b/tools/testing/selftests/powerpc/ptrace/ptrace-gpr.c
index 0b4ebcc2f485..ca29fafeed5d 100644
--- a/tools/testing/selftests/powerpc/ptrace/ptrace-gpr.c
+++ b/tools/testing/selftests/powerpc/ptrace/ptrace-gpr.c
@@ -31,7 +31,7 @@ void gpr(void)
 		ASM_LOAD_GPR_IMMED(gpr_1)
 		ASM_LOAD_FPR_SINGLE_PRECISION(flt_1)
 		:
-		: [gpr_1]"i"(GPR_1), [flt_1] "r" (&a)
+		: [gpr_1]"i"(GPR_1), [flt_1] "b" (&a)
 		: "memory", "r6", "r7", "r8", "r9", "r10",
 		"r11", "r12", "r13", "r14", "r15", "r16", "r17",
 		"r18", "r19", "r20", "r21", "r22", "r23", "r24",
diff --git a/tools/testing/selftests/powerpc/ptrace/ptrace-tm-gpr.c b/tools/testing/selftests/powerpc/ptrace/ptrace-tm-gpr.c
index 59206b96e98a..a08a91594dbe 100644
--- a/tools/testing/selftests/powerpc/ptrace/ptrace-tm-gpr.c
+++ b/tools/testing/selftests/powerpc/ptrace/ptrace-tm-gpr.c
@@ -59,8 +59,8 @@ void tm_gpr(void)
 		"3: ;"
 		: [res] "=r" (result), [texasr] "=r" (texasr)
 		: [gpr_1]"i"(GPR_1), [gpr_2]"i"(GPR_2),
-		[sprn_texasr] "i" (SPRN_TEXASR), [flt_1] "r" (&a),
-		[flt_2] "r" (&b), [cptr1] "r" (&cptr[1])
+		[sprn_texasr] "i" (SPRN_TEXASR), [flt_1] "b" (&a),
+		[flt_2] "b" (&b), [cptr1] "b" (&cptr[1])
 		: "memory", "r7", "r8", "r9", "r10",
 		"r11", "r12", "r13", "r14", "r15", "r16",
 		"r17", "r18", "r19", "r20", "r21", "r22",
diff --git a/tools/testing/selftests/powerpc/ptrace/ptrace-tm-spd-tar.c b/tools/testing/selftests/powerpc/ptrace/ptrace-tm-spd-tar.c
index b3c061dc9512..f47174746231 100644
--- a/tools/testing/selftests/powerpc/ptrace/ptrace-tm-spd-tar.c
+++ b/tools/testing/selftests/powerpc/ptrace/ptrace-tm-spd-tar.c
@@ -72,7 +72,7 @@ void tm_spd_tar(void)
 		"3: ;"
 
 		: [res] "=r" (result), [texasr] "=r" (texasr)
-		: [val] "r" (cptr[1]), [sprn_dscr]"i"(SPRN_DSCR),
+		: [sprn_dscr]"i"(SPRN_DSCR),
 		[sprn_tar]"i"(SPRN_TAR), [sprn_ppr]"i"(SPRN_PPR),
 		[sprn_texasr]"i"(SPRN_TEXASR), [tar_1]"i"(TAR_1),
 		[dscr_1]"i"(DSCR_1), [tar_2]"i"(TAR_2), [dscr_2]"i"(DSCR_2),
diff --git a/tools/testing/selftests/powerpc/ptrace/ptrace-tm-spd-vsx.c b/tools/testing/selftests/powerpc/ptrace/ptrace-tm-spd-vsx.c
index 277dade1b382..18a685bf6a09 100644
--- a/tools/testing/selftests/powerpc/ptrace/ptrace-tm-spd-vsx.c
+++ b/tools/testing/selftests/powerpc/ptrace/ptrace-tm-spd-vsx.c
@@ -77,8 +77,7 @@ void tm_spd_vsx(void)
 
 		"3: ;"
 		: [res] "=r" (result), [texasr] "=r" (texasr)
-		: [fp_load] "r" (fp_load), [fp_load_ckpt] "r" (fp_load_ckpt),
-		[sprn_texasr] "i"  (SPRN_TEXASR)
+		: [sprn_texasr] "i"  (SPRN_TEXASR)
 		: "memory", "r0", "r1", "r3", "r4",
 		"r7", "r8", "r9", "r10", "r11"
 		);
diff --git a/tools/testing/selftests/powerpc/ptrace/ptrace-tm-spr.c b/tools/testing/selftests/powerpc/ptrace/ptrace-tm-spr.c
index 51427a2465f6..ba04999254e3 100644
--- a/tools/testing/selftests/powerpc/ptrace/ptrace-tm-spr.c
+++ b/tools/testing/selftests/powerpc/ptrace/ptrace-tm-spr.c
@@ -74,7 +74,7 @@ void tm_spr(void)
 
 		"3: ;"
 		: [tfhar] "=r" (tfhar), [res] "=r" (result),
-		[texasr] "=r" (texasr), [cptr1] "=r" (cptr1)
+		[texasr] "=r" (texasr), [cptr1] "=b" (cptr1)
 		: [sprn_texasr] "i"  (SPRN_TEXASR)
 		: "memory", "r0", "r8", "r31"
 		);
diff --git a/tools/testing/selftests/powerpc/ptrace/ptrace-tm-tar.c b/tools/testing/selftests/powerpc/ptrace/ptrace-tm-tar.c
index 48b462f75023..f70023b25e6e 100644
--- a/tools/testing/selftests/powerpc/ptrace/ptrace-tm-tar.c
+++ b/tools/testing/selftests/powerpc/ptrace/ptrace-tm-tar.c
@@ -65,7 +65,7 @@ void tm_tar(void)
 		: [sprn_dscr]"i"(SPRN_DSCR), [sprn_tar]"i"(SPRN_TAR),
 		[sprn_ppr]"i"(SPRN_PPR), [sprn_texasr]"i"(SPRN_TEXASR),
 		[tar_1]"i"(TAR_1), [dscr_1]"i"(DSCR_1), [tar_2]"i"(TAR_2),
-		[dscr_2]"i"(DSCR_2), [cptr1] "r" (&cptr[1])
+		[dscr_2]"i"(DSCR_2), [cptr1] "b" (&cptr[1])
 		: "memory", "r0", "r1", "r3", "r4", "r5", "r6"
 		);
 
diff --git a/tools/testing/selftests/powerpc/ptrace/ptrace-tm-vsx.c b/tools/testing/selftests/powerpc/ptrace/ptrace-tm-vsx.c
index 17c23cabac3e..dfba80058977 100644
--- a/tools/testing/selftests/powerpc/ptrace/ptrace-tm-vsx.c
+++ b/tools/testing/selftests/powerpc/ptrace/ptrace-tm-vsx.c
@@ -65,8 +65,7 @@ void tm_vsx(void)
 
 		"3: ;"
 		: [res] "=r" (result), [texasr] "=r" (texasr)
-		: [fp_load] "r" (fp_load), [fp_load_ckpt] "r" (fp_load_ckpt),
-		[sprn_texasr] "i"  (SPRN_TEXASR), [cptr1] "r" (&cptr[1])
+		: [sprn_texasr] "i"  (SPRN_TEXASR), [cptr1] "b" (&cptr[1])
 		: "memory", "r0", "r1", "r3", "r4",
 		"r7", "r8", "r9", "r10", "r11"
 		);
-- 
2.19.0

[PATCH 2/2] selftests/powerpc: Skip test instead of failing

[Breno Leitao]

Current core-pkey selftest fails if the test runs without privileges to
write into the core pattern file (/proc/sys/kernel/core_pattern). This
causes the test to fail and give the impression that the subsystem being
tested is broken, when, in fact, the test is being executed without the
proper privileges. This is the current error:

	test: core_pkey
	tags: git_version:v4.19-3-g9e3363be9bce-dirty
	Error writing to core_pattern file: Permission denied
	failure: core_pkey

This patch simply skips this test if it runs without the proper privileges,
avoiding this undesired failure.

CC: Thiago Jung Bauermann <bauerman@linux.ibm.com>
Signed-off-by: Breno Leitao <leitao@debian.org>
---
 tools/testing/selftests/powerpc/ptrace/core-pkey.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/tools/testing/selftests/powerpc/ptrace/core-pkey.c b/tools/testing/selftests/powerpc/ptrace/core-pkey.c
index e23e2e199eb4..e07949120fc8 100644
--- a/tools/testing/selftests/powerpc/ptrace/core-pkey.c
+++ b/tools/testing/selftests/powerpc/ptrace/core-pkey.c
@@ -352,17 +352,11 @@ static int write_core_pattern(const char *core_pattern)
 	FILE *f;
 
 	f = fopen(core_pattern_file, "w");
-	if (!f) {
-		perror("Error writing to core_pattern file");
-		return TEST_FAIL;
-	}
+	SKIP_IF(!f);
 
 	ret = fwrite(core_pattern, 1, len, f);
 	fclose(f);
-	if (ret != len) {
-		perror("Error writing to core_pattern file");
-		return TEST_FAIL;
-	}
+	SKIP_IF(ret != len);
 
 	return TEST_PASS;
 }
-- 
2.19.0

Re: [PATCH 2/2] selftests/powerpc: Skip test instead of failing

[Tyrel Datwyler]

On 10/23/2018 01:23 PM, Breno Leitao wrote:
> Current core-pkey selftest fails if the test runs without privileges to
> write into the core pattern file (/proc/sys/kernel/core_pattern). This
> causes the test to fail and give the impression that the subsystem being
> tested is broken, when, in fact, the test is being executed without the
> proper privileges. This is the current error:
> 
> 	test: core_pkey
> 	tags: git_version:v4.19-3-g9e3363be9bce-dirty
> 	Error writing to core_pattern file: Permission denied
> 	failure: core_pkey
> 
> This patch simply skips this test if it runs without the proper privileges,
> avoiding this undesired failure.
> 
> CC: Thiago Jung Bauermann <bauerman@linux.ibm.com>
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
>  tools/testing/selftests/powerpc/ptrace/core-pkey.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/testing/selftests/powerpc/ptrace/core-pkey.c b/tools/testing/selftests/powerpc/ptrace/core-pkey.c
> index e23e2e199eb4..e07949120fc8 100644
> --- a/tools/testing/selftests/powerpc/ptrace/core-pkey.c
> +++ b/tools/testing/selftests/powerpc/ptrace/core-pkey.c
> @@ -352,17 +352,11 @@ static int write_core_pattern(const char *core_pattern)
>  	FILE *f;
> 
>  	f = fopen(core_pattern_file, "w");
> -	if (!f) {
> -		perror("Error writing to core_pattern file");
> -		return TEST_FAIL;
> -	}
> +	SKIP_IF(!f);
> 
>  	ret = fwrite(core_pattern, 1, len, f);
>  	fclose(f);
> -	if (ret != len) {
> -		perror("Error writing to core_pattern file");
> -		return TEST_FAIL;
> -	}
> +	SKIP_IF(ret != len);

If we don't have proper privileges we should fail on the open, right? So wouldn't we still want to fail on the write if something goes wrong?

-Tyrel

> 
>  	return TEST_PASS;
>  }
> 

Re: [PATCH 1/2] selftests/powerpc: Allocate base registers

[Segher Boessenkool]

On Tue, Oct 23, 2018 at 05:23:16PM -0300, Breno Leitao wrote:
> Some ptrace selftests are passing input operands using a constraint that
> can allocate any register for the operand, and using these registers on
> load/store operations.
> 
> If the register allocated by the compiler happens to be zero (r0), it might
> cause an invalid memory address access, since load and store operations
> consider the content of 0x0 address if the base register is r0, instead
> of the content of the r0 register. For example:
> 
> 	r1 := 0xdeadbeef
> 	r0 := 0xdeadbeef
> 	
> 	ld r2, 0(r1) /* will load into r2 the content of r1 address */
> 	ld r2, 0(r0) /* will load into r2 the context of 0x0 */

That isn't valid syntax: you have to write
	ld r2, 0(0)

(s/context/content/ btw)

> In order to avoid this possible problem, the inline assembly constraint
> should be aware that these registers will be used as a base register, thus,
> r0 should not be alocated.

(allocated)

The patch looks fine :-)

Reviewed-by: Segher Boessenkool <segher@kernel.crashing.org>


Segher

Re: [PATCH 2/2] selftests/powerpc: Skip test instead of failing

[Breno Leitao]

Hi Tyrel,

On 10/23/2018 05:41 PM, Tyrel Datwyler wrote:
>> --- a/tools/testing/selftests/powerpc/ptrace/core-pkey.c
>> +++ b/tools/testing/selftests/powerpc/ptrace/core-pkey.c
>> @@ -352,17 +352,11 @@ static int write_core_pattern(const char *core_pattern)
>>  	FILE *f;
>>
>>  	f = fopen(core_pattern_file, "w");
>> -	if (!f) {
>> -		perror("Error writing to core_pattern file");
>> -		return TEST_FAIL;
>> -	}
>> +	SKIP_IF(!f);
>>
>>  	ret = fwrite(core_pattern, 1, len, f);
>>  	fclose(f);
>> -	if (ret != len) {
>> -		perror("Error writing to core_pattern file");
>> -		return TEST_FAIL;
>> -	}
>> +	SKIP_IF(ret != len);

> If we don't have proper privileges we should fail on the open, right?
> So wouldn't we still want to fail on the write if something goes
> wrong?

That is a good point. Should the test fail or skip if it is not possible
to create the infrastructure to run the core test?

Trying to find the answer in the current test sets, I find tests where
the self test skips if the test environment is not able to be set up, as
for example, when a memory allocation fails.

File: tools/testing/selftests/powerpc/alignment/alignment_handler.c

        ci1 = mmap(NULL, bufsize, PROT_WRITE, MAP_SHARED,
                   fd, bufsize);
        if ((ci0 == MAP_FAILED) || (ci1 == MAP_FAILED)) {
                printf("\n");
                perror("mmap failed");
                SKIP_IF(1);
        }

Re: [PATCH 2/2] selftests/powerpc: Skip test instead of failing

[Thiago Jung Bauermann]

Breno Leitao <leitao@debian.org> writes:

> Hi Tyrel,
>
> On 10/23/2018 05:41 PM, Tyrel Datwyler wrote:
>>> --- a/tools/testing/selftests/powerpc/ptrace/core-pkey.c
>>> +++ b/tools/testing/selftests/powerpc/ptrace/core-pkey.c
>>> @@ -352,17 +352,11 @@ static int write_core_pattern(const char *core_pattern)
>>>  	FILE *f;
>>>
>>>  	f = fopen(core_pattern_file, "w");
>>> -	if (!f) {
>>> -		perror("Error writing to core_pattern file");
>>> -		return TEST_FAIL;
>>> -	}
>>> +	SKIP_IF(!f);
>>>
>>>  	ret = fwrite(core_pattern, 1, len, f);
>>>  	fclose(f);
>>> -	if (ret != len) {
>>> -		perror("Error writing to core_pattern file");
>>> -		return TEST_FAIL;
>>> -	}
>>> +	SKIP_IF(ret != len);
>
>> If we don't have proper privileges we should fail on the open, right?
>> So wouldn't we still want to fail on the write if something goes
>> wrong?
>
> That is a good point. Should the test fail or skip if it is not possible
> to create the infrastructure to run the core test?
>
> Trying to find the answer in the current test sets, I find tests where
> the self test skips if the test environment is not able to be set up, as
> for example, when a memory allocation fails.
>
> File: tools/testing/selftests/powerpc/alignment/alignment_handler.c
>
>         ci1 = mmap(NULL, bufsize, PROT_WRITE, MAP_SHARED,
>                    fd, bufsize);
>         if ((ci0 == MAP_FAILED) || (ci1 == MAP_FAILED)) {
>                 printf("\n");
>                 perror("mmap failed");
>                 SKIP_IF(1);
>         }

I think TEST_FAIL means the test was able to exercise the feature
and found a problem with it. In this case, the test wasn't able to
exercise the feature so it's not appropriate.

Ideally, there should be a TEST_ERROR result for a case like this where
an unexpected problem prevented the testcase from exercising the
feature.

If we're to use the an existing result then I vote for SKIP_IF.

For reference, here are the test results that DejaGnu supports (it is
the test harness used by some GNU projects):

https://www.gnu.org/software/dejagnu/manual/Output-States.html

I would say that SKIP_IF corresponds to UNSUPPORTED in DejaGnu.

--
Thiago Jung Bauermann
IBM Linux Technology Center

Re: [PATCH 2/2] selftests/powerpc: Skip test instead of failing

[Michael Ellerman]

Thiago Jung Bauermann <bauerman@linux.ibm.com> writes:

> Breno Leitao <leitao@debian.org> writes:
>
>> Hi Tyrel,
>>
>> On 10/23/2018 05:41 PM, Tyrel Datwyler wrote:
>>>> --- a/tools/testing/selftests/powerpc/ptrace/core-pkey.c
>>>> +++ b/tools/testing/selftests/powerpc/ptrace/core-pkey.c
>>>> @@ -352,17 +352,11 @@ static int write_core_pattern(const char *core_pattern)
>>>>  	FILE *f;
>>>>
>>>>  	f = fopen(core_pattern_file, "w");
>>>> -	if (!f) {
>>>> -		perror("Error writing to core_pattern file");
>>>> -		return TEST_FAIL;
>>>> -	}
>>>> +	SKIP_IF(!f);
>>>>
>>>>  	ret = fwrite(core_pattern, 1, len, f);
>>>>  	fclose(f);
>>>> -	if (ret != len) {
>>>> -		perror("Error writing to core_pattern file");
>>>> -		return TEST_FAIL;
>>>> -	}
>>>> +	SKIP_IF(ret != len);
>>
>>> If we don't have proper privileges we should fail on the open, right?
>>> So wouldn't we still want to fail on the write if something goes
>>> wrong?
>>
>> That is a good point. Should the test fail or skip if it is not possible
>> to create the infrastructure to run the core test?
>>
>> Trying to find the answer in the current test sets, I find tests where
>> the self test skips if the test environment is not able to be set up, as
>> for example, when a memory allocation fails.
>>
>> File: tools/testing/selftests/powerpc/alignment/alignment_handler.c
>>
>>         ci1 = mmap(NULL, bufsize, PROT_WRITE, MAP_SHARED,
>>                    fd, bufsize);
>>         if ((ci0 == MAP_FAILED) || (ci1 == MAP_FAILED)) {
>>                 printf("\n");
>>                 perror("mmap failed");
>>                 SKIP_IF(1);
>>         }
>
> I think TEST_FAIL means the test was able to exercise the feature
> and found a problem with it. In this case, the test wasn't able to
> exercise the feature so it's not appropriate.
>
> Ideally, there should be a TEST_ERROR result for a case like this where
> an unexpected problem prevented the testcase from exercising the
> feature.
>
> If we're to use the an existing result then I vote for SKIP_IF.

Yeah I agree.

See for example some of the TM tests, which skip if TM is not available.
Or the alignment test which skips if it can't open /dev/fb0.

In this case it should print "you need to be root to run this" and then
skip.

cheers

Re: [PATCH 2/2] selftests/powerpc: Skip test instead of failing

[Tyrel Datwyler]

On 10/30/2018 08:16 AM, Michael Ellerman wrote:
> Thiago Jung Bauermann <bauerman@linux.ibm.com> writes:
> 
>> Breno Leitao <leitao@debian.org> writes:
>>
>>> Hi Tyrel,
>>>
>>> On 10/23/2018 05:41 PM, Tyrel Datwyler wrote:
>>>>> --- a/tools/testing/selftests/powerpc/ptrace/core-pkey.c
>>>>> +++ b/tools/testing/selftests/powerpc/ptrace/core-pkey.c
>>>>> @@ -352,17 +352,11 @@ static int write_core_pattern(const char *core_pattern)
>>>>>  	FILE *f;
>>>>>
>>>>>  	f = fopen(core_pattern_file, "w");
>>>>> -	if (!f) {
>>>>> -		perror("Error writing to core_pattern file");
>>>>> -		return TEST_FAIL;
>>>>> -	}
>>>>> +	SKIP_IF(!f);
>>>>>
>>>>>  	ret = fwrite(core_pattern, 1, len, f);
>>>>>  	fclose(f);
>>>>> -	if (ret != len) {
>>>>> -		perror("Error writing to core_pattern file");
>>>>> -		return TEST_FAIL;
>>>>> -	}
>>>>> +	SKIP_IF(ret != len);
>>>
>>>> If we don't have proper privileges we should fail on the open, right?
>>>> So wouldn't we still want to fail on the write if something goes
>>>> wrong?
>>>
>>> That is a good point. Should the test fail or skip if it is not possible
>>> to create the infrastructure to run the core test?
>>>
>>> Trying to find the answer in the current test sets, I find tests where
>>> the self test skips if the test environment is not able to be set up, as
>>> for example, when a memory allocation fails.
>>>
>>> File: tools/testing/selftests/powerpc/alignment/alignment_handler.c
>>>
>>>         ci1 = mmap(NULL, bufsize, PROT_WRITE, MAP_SHARED,
>>>                    fd, bufsize);
>>>         if ((ci0 == MAP_FAILED) || (ci1 == MAP_FAILED)) {
>>>                 printf("\n");
>>>                 perror("mmap failed");
>>>                 SKIP_IF(1);
>>>         }
>>
>> I think TEST_FAIL means the test was able to exercise the feature
>> and found a problem with it. In this case, the test wasn't able to
>> exercise the feature so it's not appropriate.
>>
>> Ideally, there should be a TEST_ERROR result for a case like this where
>> an unexpected problem prevented the testcase from exercising the
>> feature.
>>
>> If we're to use the an existing result then I vote for SKIP_IF.
> 
> Yeah I agree.
> 
> See for example some of the TM tests, which skip if TM is not available.
> Or the alignment test which skips if it can't open /dev/fb0.
> 
> In this case it should print "you need to be root to run this" and then
> skip.

Agreed that there should be some indicator of why we are skipping the test. My original point I was trying to make was that I thought skipping a failed open was okay because we are likely not root. However, I wasn't sure that a failed write was okay to skip as that could be an indicator that something has actually been broken.

-Tyrel

> 
> cheers
> 

Re: [PATCH 2/2] selftests/powerpc: Skip test instead of failing

[Michael Ellerman]

Tyrel Datwyler <tyreld@linux.vnet.ibm.com> writes:
> On 10/23/2018 01:23 PM, Breno Leitao wrote:
>> Current core-pkey selftest fails if the test runs without privileges to
>> write into the core pattern file (/proc/sys/kernel/core_pattern). This
>> causes the test to fail and give the impression that the subsystem being
>> tested is broken, when, in fact, the test is being executed without the
>> proper privileges. This is the current error:
>> 
>> 	test: core_pkey
>> 	tags: git_version:v4.19-3-g9e3363be9bce-dirty
>> 	Error writing to core_pattern file: Permission denied
>> 	failure: core_pkey
>> 
>> This patch simply skips this test if it runs without the proper privileges,
>> avoiding this undesired failure.
>> 
>> CC: Thiago Jung Bauermann <bauerman@linux.ibm.com>
>> Signed-off-by: Breno Leitao <leitao@debian.org>
>> ---
>>  tools/testing/selftests/powerpc/ptrace/core-pkey.c | 10 ++--------
>>  1 file changed, 2 insertions(+), 8 deletions(-)
>> 
>> diff --git a/tools/testing/selftests/powerpc/ptrace/core-pkey.c b/tools/testing/selftests/powerpc/ptrace/core-pkey.c
>> index e23e2e199eb4..e07949120fc8 100644
>> --- a/tools/testing/selftests/powerpc/ptrace/core-pkey.c
>> +++ b/tools/testing/selftests/powerpc/ptrace/core-pkey.c
>> @@ -352,17 +352,11 @@ static int write_core_pattern(const char *core_pattern)
>>  	FILE *f;
>> 
>>  	f = fopen(core_pattern_file, "w");
>> -	if (!f) {
>> -		perror("Error writing to core_pattern file");
>> -		return TEST_FAIL;
>> -	}
>> +	SKIP_IF(!f);
>> 
>>  	ret = fwrite(core_pattern, 1, len, f);
>>  	fclose(f);
>> -	if (ret != len) {
>> -		perror("Error writing to core_pattern file");
>> -		return TEST_FAIL;
>> -	}
>> +	SKIP_IF(ret != len);
>
> If we don't have proper privileges we should fail on the open, right?
> So wouldn't we still want to fail on the write if something goes
> wrong?

Yes you're right. If we don't have permission then the open should have
failed, and we skip then.

But if the open succeeded and the write fails then we don't know what's
going on and the test should fail.

cheers

Re: [PATCH 2/2] selftests/powerpc: Skip test instead of failing

[Michael Ellerman]

Tyrel Datwyler <tyreld@linux.vnet.ibm.com> writes:
> On 10/30/2018 08:16 AM, Michael Ellerman wrote:
>> Thiago Jung Bauermann <bauerman@linux.ibm.com> writes:
>> 
>>> Breno Leitao <leitao@debian.org> writes:
>>>
>>>> Hi Tyrel,
>>>>
>>>> On 10/23/2018 05:41 PM, Tyrel Datwyler wrote:
>>>>>> --- a/tools/testing/selftests/powerpc/ptrace/core-pkey.c
>>>>>> +++ b/tools/testing/selftests/powerpc/ptrace/core-pkey.c
>>>>>> @@ -352,17 +352,11 @@ static int write_core_pattern(const char *core_pattern)
>>>>>>  	FILE *f;
>>>>>>
>>>>>>  	f = fopen(core_pattern_file, "w");
>>>>>> -	if (!f) {
>>>>>> -		perror("Error writing to core_pattern file");
>>>>>> -		return TEST_FAIL;
>>>>>> -	}
>>>>>> +	SKIP_IF(!f);
>>>>>>
>>>>>>  	ret = fwrite(core_pattern, 1, len, f);
>>>>>>  	fclose(f);
>>>>>> -	if (ret != len) {
>>>>>> -		perror("Error writing to core_pattern file");
>>>>>> -		return TEST_FAIL;
>>>>>> -	}
>>>>>> +	SKIP_IF(ret != len);
>>>>
>>>>> If we don't have proper privileges we should fail on the open, right?
>>>>> So wouldn't we still want to fail on the write if something goes
>>>>> wrong?
>>>>
>>>> That is a good point. Should the test fail or skip if it is not possible
>>>> to create the infrastructure to run the core test?
>>>>
>>>> Trying to find the answer in the current test sets, I find tests where
>>>> the self test skips if the test environment is not able to be set up, as
>>>> for example, when a memory allocation fails.
>>>>
>>>> File: tools/testing/selftests/powerpc/alignment/alignment_handler.c
>>>>
>>>>         ci1 = mmap(NULL, bufsize, PROT_WRITE, MAP_SHARED,
>>>>                    fd, bufsize);
>>>>         if ((ci0 == MAP_FAILED) || (ci1 == MAP_FAILED)) {
>>>>                 printf("\n");
>>>>                 perror("mmap failed");
>>>>                 SKIP_IF(1);
>>>>         }
>>>
>>> I think TEST_FAIL means the test was able to exercise the feature
>>> and found a problem with it. In this case, the test wasn't able to
>>> exercise the feature so it's not appropriate.
>>>
>>> Ideally, there should be a TEST_ERROR result for a case like this where
>>> an unexpected problem prevented the testcase from exercising the
>>> feature.
>>>
>>> If we're to use the an existing result then I vote for SKIP_IF.
>> 
>> Yeah I agree.
>> 
>> See for example some of the TM tests, which skip if TM is not available.
>> Or the alignment test which skips if it can't open /dev/fb0.
>> 
>> In this case it should print "you need to be root to run this" and then
>> skip.
>
> Agreed that there should be some indicator of why we are skipping the
> test. My original point I was trying to make was that I thought
> skipping a failed open was okay because we are likely not root.
> However, I wasn't sure that a failed write was okay to skip as that
> could be an indicator that something has actually been broken.

Yes I agree. I didn't read your mail closely enough. Have just replied
to it.

cheers