Tyrel Datwyler <tyr...@linux.vnet.ibm.com> writes:
> 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.
Yes I agree. I didn't read your mail closely enough. Have just replied
to it.
cheers
