| Message ID | 20240518001655.work.053-kees@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | selftests: rtc: rtctest: Do not open-code TEST_HARNESS_MAIN | expand |
On 17/05/2024 17:16:58-0700, Kees Cook wrote: > Argument processing is specific to the test harness code. Any optional > information needs to be passed via environment variables. Move alternate > path to the RTC_DEV environment variable. Also do not open-code > TEST_HARNESS_MAIN because its definition may change. Th main issue doing that is that this breaks the main use case of rtctest as /dev/rtc1 is usually the main target for those tests. Having the RTC_DEV environment variable only documented n this commit message is definitively not enough, I'm going to have to handle zillion of complaints that this is not working anymore. > > Additionally, setup checking can be done in the FIXTURE_SETUP(). With > this adjustment, also improve the error reporting when the device cannot > be opened. > > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > Cc: Alexandre Belloni <alexandre.belloni@bootlin.com> > Cc: Shuah Khan <shuah@kernel.org> > Cc: Masahiro Yamada <masahiroy@kernel.org> > Cc: linux-rtc@vger.kernel.org > Cc: linux-kselftest@vger.kernel.org > --- > tools/testing/selftests/rtc/Makefile | 2 +- > tools/testing/selftests/rtc/rtctest.c | 66 +++++---------------------- > 2 files changed, 13 insertions(+), 55 deletions(-) > > diff --git a/tools/testing/selftests/rtc/Makefile b/tools/testing/selftests/rtc/Makefile > index 55198ecc04db..654f9d58da3c 100644 > --- a/tools/testing/selftests/rtc/Makefile > +++ b/tools/testing/selftests/rtc/Makefile > @@ -1,5 +1,5 @@ > # SPDX-License-Identifier: GPL-2.0 > -CFLAGS += -O3 -Wl,-no-as-needed -Wall > +CFLAGS += -O3 -Wl,-no-as-needed -Wall $(KHDR_INCLUDES) > LDLIBS += -lrt -lpthread -lm > > TEST_GEN_PROGS = rtctest > diff --git a/tools/testing/selftests/rtc/rtctest.c b/tools/testing/selftests/rtc/rtctest.c > index 63ce02d1d5cc..41cfefcc20e1 100644 > --- a/tools/testing/selftests/rtc/rtctest.c > +++ b/tools/testing/selftests/rtc/rtctest.c > @@ -30,7 +30,18 @@ FIXTURE(rtc) { > }; > > FIXTURE_SETUP(rtc) { > + char *alternate = getenv("RTC_DEV"); > + > + if (alternate) > + rtc_file = alternate; > + > self->fd = open(rtc_file, O_RDONLY); > + > + if (self->fd == -1 && errno == ENOENT) > + SKIP(return, "Skipping test since %s does not exist", rtc_file); > + EXPECT_NE(-1, self->fd) { > + TH_LOG("%s: %s\n", rtc_file, strerror(errno)); > + } > } > > FIXTURE_TEARDOWN(rtc) { > @@ -41,10 +52,6 @@ TEST_F(rtc, date_read) { > int rc; > struct rtc_time rtc_tm; > > - if (self->fd == -1 && errno == ENOENT) > - SKIP(return, "Skipping test since %s does not exist", rtc_file); > - ASSERT_NE(-1, self->fd); > - > /* Read the RTC time/date */ > rc = ioctl(self->fd, RTC_RD_TIME, &rtc_tm); > ASSERT_NE(-1, rc); > @@ -88,10 +95,6 @@ TEST_F_TIMEOUT(rtc, date_read_loop, READ_LOOP_DURATION_SEC + 2) { > struct rtc_time rtc_tm; > time_t start_rtc_read, prev_rtc_read; > > - if (self->fd == -1 && errno == ENOENT) > - SKIP(return, "Skipping test since %s does not exist", rtc_file); > - ASSERT_NE(-1, self->fd); > - > TH_LOG("Continuously reading RTC time for %ds (with %dms breaks after every read).", > READ_LOOP_DURATION_SEC, READ_LOOP_SLEEP_MS); > > @@ -126,10 +129,6 @@ TEST_F_TIMEOUT(rtc, uie_read, NUM_UIE + 2) { > int i, rc, irq = 0; > unsigned long data; > > - if (self->fd == -1 && errno == ENOENT) > - SKIP(return, "Skipping test since %s does not exist", rtc_file); > - ASSERT_NE(-1, self->fd); > - > /* Turn on update interrupts */ > rc = ioctl(self->fd, RTC_UIE_ON, 0); > if (rc == -1) { > @@ -155,10 +154,6 @@ TEST_F(rtc, uie_select) { > int i, rc, irq = 0; > unsigned long data; > > - if (self->fd == -1 && errno == ENOENT) > - SKIP(return, "Skipping test since %s does not exist", rtc_file); > - ASSERT_NE(-1, self->fd); > - > /* Turn on update interrupts */ > rc = ioctl(self->fd, RTC_UIE_ON, 0); > if (rc == -1) { > @@ -198,10 +193,6 @@ TEST_F(rtc, alarm_alm_set) { > time_t secs, new; > int rc; > > - if (self->fd == -1 && errno == ENOENT) > - SKIP(return, "Skipping test since %s does not exist", rtc_file); > - ASSERT_NE(-1, self->fd); > - > rc = ioctl(self->fd, RTC_RD_TIME, &tm); > ASSERT_NE(-1, rc); > > @@ -256,10 +247,6 @@ TEST_F(rtc, alarm_wkalm_set) { > time_t secs, new; > int rc; > > - if (self->fd == -1 && errno == ENOENT) > - SKIP(return, "Skipping test since %s does not exist", rtc_file); > - ASSERT_NE(-1, self->fd); > - > rc = ioctl(self->fd, RTC_RD_TIME, &alarm.time); > ASSERT_NE(-1, rc); > > @@ -308,10 +295,6 @@ TEST_F_TIMEOUT(rtc, alarm_alm_set_minute, 65) { > time_t secs, new; > int rc; > > - if (self->fd == -1 && errno == ENOENT) > - SKIP(return, "Skipping test since %s does not exist", rtc_file); > - ASSERT_NE(-1, self->fd); > - > rc = ioctl(self->fd, RTC_RD_TIME, &tm); > ASSERT_NE(-1, rc); > > @@ -366,10 +349,6 @@ TEST_F_TIMEOUT(rtc, alarm_wkalm_set_minute, 65) { > time_t secs, new; > int rc; > > - if (self->fd == -1 && errno == ENOENT) > - SKIP(return, "Skipping test since %s does not exist", rtc_file); > - ASSERT_NE(-1, self->fd); > - > rc = ioctl(self->fd, RTC_RD_TIME, &alarm.time); > ASSERT_NE(-1, rc); > > @@ -410,25 +389,4 @@ TEST_F_TIMEOUT(rtc, alarm_wkalm_set_minute, 65) { > ASSERT_EQ(new, secs); > } > > -static void __attribute__((constructor)) > -__constructor_order_last(void) > -{ > - if (!__constructor_order) > - __constructor_order = _CONSTRUCTOR_ORDER_BACKWARD; > -} > - > -int main(int argc, char **argv) > -{ > - switch (argc) { > - case 2: > - rtc_file = argv[1]; > - /* FALLTHROUGH */ > - case 1: > - break; > - default: > - fprintf(stderr, "usage: %s [rtcdev]\n", argv[0]); > - return 1; > - } > - > - return test_harness_run(argc, argv); > -} > +TEST_HARNESS_MAIN > -- > 2.34.1 >
On Sat, May 18, 2024 at 10:23:54PM +0200, Alexandre Belloni wrote: > On 17/05/2024 17:16:58-0700, Kees Cook wrote: > > Argument processing is specific to the test harness code. Any optional > > information needs to be passed via environment variables. Move alternate > > path to the RTC_DEV environment variable. Also do not open-code > > TEST_HARNESS_MAIN because its definition may change. > > Th main issue doing that is that this breaks the main use case of > rtctest as /dev/rtc1 is usually the main target for those tests. Having > the RTC_DEV environment variable only documented n this commit message > is definitively not enough, I'm going to have to handle zillion of > complaints that this is not working anymore. Hm, maybe switch the default to /dev/rtc1? Maybe there's a better way to integrate arguments into a test runner. Right now the core harness code is doing the argument parsing...
diff --git a/tools/testing/selftests/rtc/Makefile b/tools/testing/selftests/rtc/Makefile index 55198ecc04db..654f9d58da3c 100644 --- a/tools/testing/selftests/rtc/Makefile +++ b/tools/testing/selftests/rtc/Makefile @@ -1,5 +1,5 @@ # SPDX-License-Identifier: GPL-2.0 -CFLAGS += -O3 -Wl,-no-as-needed -Wall +CFLAGS += -O3 -Wl,-no-as-needed -Wall $(KHDR_INCLUDES) LDLIBS += -lrt -lpthread -lm TEST_GEN_PROGS = rtctest diff --git a/tools/testing/selftests/rtc/rtctest.c b/tools/testing/selftests/rtc/rtctest.c index 63ce02d1d5cc..41cfefcc20e1 100644 --- a/tools/testing/selftests/rtc/rtctest.c +++ b/tools/testing/selftests/rtc/rtctest.c @@ -30,7 +30,18 @@ FIXTURE(rtc) { }; FIXTURE_SETUP(rtc) { + char *alternate = getenv("RTC_DEV"); + + if (alternate) + rtc_file = alternate; + self->fd = open(rtc_file, O_RDONLY); + + if (self->fd == -1 && errno == ENOENT) + SKIP(return, "Skipping test since %s does not exist", rtc_file); + EXPECT_NE(-1, self->fd) { + TH_LOG("%s: %s\n", rtc_file, strerror(errno)); + } } FIXTURE_TEARDOWN(rtc) { @@ -41,10 +52,6 @@ TEST_F(rtc, date_read) { int rc; struct rtc_time rtc_tm; - if (self->fd == -1 && errno == ENOENT) - SKIP(return, "Skipping test since %s does not exist", rtc_file); - ASSERT_NE(-1, self->fd); - /* Read the RTC time/date */ rc = ioctl(self->fd, RTC_RD_TIME, &rtc_tm); ASSERT_NE(-1, rc); @@ -88,10 +95,6 @@ TEST_F_TIMEOUT(rtc, date_read_loop, READ_LOOP_DURATION_SEC + 2) { struct rtc_time rtc_tm; time_t start_rtc_read, prev_rtc_read; - if (self->fd == -1 && errno == ENOENT) - SKIP(return, "Skipping test since %s does not exist", rtc_file); - ASSERT_NE(-1, self->fd); - TH_LOG("Continuously reading RTC time for %ds (with %dms breaks after every read).", READ_LOOP_DURATION_SEC, READ_LOOP_SLEEP_MS); @@ -126,10 +129,6 @@ TEST_F_TIMEOUT(rtc, uie_read, NUM_UIE + 2) { int i, rc, irq = 0; unsigned long data; - if (self->fd == -1 && errno == ENOENT) - SKIP(return, "Skipping test since %s does not exist", rtc_file); - ASSERT_NE(-1, self->fd); - /* Turn on update interrupts */ rc = ioctl(self->fd, RTC_UIE_ON, 0); if (rc == -1) { @@ -155,10 +154,6 @@ TEST_F(rtc, uie_select) { int i, rc, irq = 0; unsigned long data; - if (self->fd == -1 && errno == ENOENT) - SKIP(return, "Skipping test since %s does not exist", rtc_file); - ASSERT_NE(-1, self->fd); - /* Turn on update interrupts */ rc = ioctl(self->fd, RTC_UIE_ON, 0); if (rc == -1) { @@ -198,10 +193,6 @@ TEST_F(rtc, alarm_alm_set) { time_t secs, new; int rc; - if (self->fd == -1 && errno == ENOENT) - SKIP(return, "Skipping test since %s does not exist", rtc_file); - ASSERT_NE(-1, self->fd); - rc = ioctl(self->fd, RTC_RD_TIME, &tm); ASSERT_NE(-1, rc); @@ -256,10 +247,6 @@ TEST_F(rtc, alarm_wkalm_set) { time_t secs, new; int rc; - if (self->fd == -1 && errno == ENOENT) - SKIP(return, "Skipping test since %s does not exist", rtc_file); - ASSERT_NE(-1, self->fd); - rc = ioctl(self->fd, RTC_RD_TIME, &alarm.time); ASSERT_NE(-1, rc); @@ -308,10 +295,6 @@ TEST_F_TIMEOUT(rtc, alarm_alm_set_minute, 65) { time_t secs, new; int rc; - if (self->fd == -1 && errno == ENOENT) - SKIP(return, "Skipping test since %s does not exist", rtc_file); - ASSERT_NE(-1, self->fd); - rc = ioctl(self->fd, RTC_RD_TIME, &tm); ASSERT_NE(-1, rc); @@ -366,10 +349,6 @@ TEST_F_TIMEOUT(rtc, alarm_wkalm_set_minute, 65) { time_t secs, new; int rc; - if (self->fd == -1 && errno == ENOENT) - SKIP(return, "Skipping test since %s does not exist", rtc_file); - ASSERT_NE(-1, self->fd); - rc = ioctl(self->fd, RTC_RD_TIME, &alarm.time); ASSERT_NE(-1, rc); @@ -410,25 +389,4 @@ TEST_F_TIMEOUT(rtc, alarm_wkalm_set_minute, 65) { ASSERT_EQ(new, secs); } -static void __attribute__((constructor)) -__constructor_order_last(void) -{ - if (!__constructor_order) - __constructor_order = _CONSTRUCTOR_ORDER_BACKWARD; -} - -int main(int argc, char **argv) -{ - switch (argc) { - case 2: - rtc_file = argv[1]; - /* FALLTHROUGH */ - case 1: - break; - default: - fprintf(stderr, "usage: %s [rtcdev]\n", argv[0]); - return 1; - } - - return test_harness_run(argc, argv); -} +TEST_HARNESS_MAIN
Argument processing is specific to the test harness code. Any optional information needs to be passed via environment variables. Move alternate path to the RTC_DEV environment variable. Also do not open-code TEST_HARNESS_MAIN because its definition may change. Additionally, setup checking can be done in the FIXTURE_SETUP(). With this adjustment, also improve the error reporting when the device cannot be opened. Signed-off-by: Kees Cook <keescook@chromium.org> --- Cc: Alexandre Belloni <alexandre.belloni@bootlin.com> Cc: Shuah Khan <shuah@kernel.org> Cc: Masahiro Yamada <masahiroy@kernel.org> Cc: linux-rtc@vger.kernel.org Cc: linux-kselftest@vger.kernel.org --- tools/testing/selftests/rtc/Makefile | 2 +- tools/testing/selftests/rtc/rtctest.c | 66 +++++---------------------- 2 files changed, 13 insertions(+), 55 deletions(-)