| Message ID | 1540326197-15537-2-git-send-email-leitao@debian.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/2] selftests/powerpc: Allocate base registers | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | next/apply_patch Successfully applied |
| snowpatch_ozlabs/checkpatch | success | Test checkpatch on branch next |
| snowpatch_ozlabs/build-ppc64le | success | Test build-ppc64le on branch next |
| snowpatch_ozlabs/build-ppc64be | success | Test build-ppc64be on branch next |
| snowpatch_ozlabs/build-ppc64e | success | Test build-ppc64e on branch next |
| snowpatch_ozlabs/build-ppc32 | success | Test build-ppc32 on branch next |
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; > } >
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); }
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
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
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 >
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
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
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; }
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(-)