On 10/30/2018 08:16 AM, Michael Ellerman wrote:
> Thiago Jung Bauermann <bauer...@linux.ibm.com> writes:
>
>> Breno Leitao <lei...@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
>
