[LTP] [PATCH v2 1/3] testcases/kernel/syscalls/ioctl: Add test for RTC ioctls used to read and set RTC time
Filip Bozuta
fbozuta1@gmail.com
Thu Apr 30 22:28:35 CEST 2020
On Thu, Apr 30, 2020 at 5:13 PM Cyril Hrubis <chrubis@suse.cz> wrote:
>
> Hi!
> > This patch tests functionalities of following ioctls:
> >
> > RTC_RD_TIME - Getting RTC time
> >
> > Returns this RTC's time in the following structure:
> >
> > struct rtc_time {
> > int tm_sec;
> > int tm_min;
> > int tm_hour;
> > int tm_mday;
> > int tm_mon;
> > int tm_year;
> > int tm_wday; /* unused */
> > int tm_yday; /* unused */
> > int tm_isdst; /* unused */
> > };
> >
> > The fields in this structure have the same meaning and
> > ranges as the tm structure described in gmtime man page.
> > A pointer to this structure should be passed as the third
> > ioctl argument.
> >
> > RTC_SET_TIME - Setting RTC time
> >
> > Sets this RTC's time to the time specified by the rtc_time
> > structure pointed to by the third ioctl argument. To set the
> > RTC's time the process must be privileged (i.e., have the
> > CAP_SYS_TIME capability).
> >
> > Signed-off-by: Filip Bozuta <Filip.Bozuta@rt-rk.com>
> > ---
> > runtest/syscalls | 2 +
> > testcases/kernel/syscalls/ioctl/.gitignore | 1 +
> > testcases/kernel/syscalls/ioctl/ioctl_rtc01.c | 121 ++++++++++++++++++
> > 3 files changed, 124 insertions(+)
> > create mode 100644 testcases/kernel/syscalls/ioctl/ioctl_rtc01.c
> >
> > diff --git a/runtest/syscalls b/runtest/syscalls
> > index 44254d7da..c6b8a85ad 100644
> > --- a/runtest/syscalls
> > +++ b/runtest/syscalls
> > @@ -539,6 +539,8 @@ ioctl_ns07 ioctl_ns07
> >
> > ioctl_sg01 ioctl_sg01
> >
> > +ioctl_rtc01 ioctl_rtc01
> > +
> > inotify_init1_01 inotify_init1_01
> > inotify_init1_02 inotify_init1_02
> >
> > diff --git a/testcases/kernel/syscalls/ioctl/.gitignore b/testcases/kernel/syscalls/ioctl/.gitignore
> > index 97fbb9681..b297407bd 100644
> > --- a/testcases/kernel/syscalls/ioctl/.gitignore
> > +++ b/testcases/kernel/syscalls/ioctl/.gitignore
> > @@ -14,3 +14,4 @@
> > /ioctl_ns06
> > /ioctl_ns07
> > /ioctl_sg01
> > +/ioctl_rtc01
> > diff --git a/testcases/kernel/syscalls/ioctl/ioctl_rtc01.c b/testcases/kernel/syscalls/ioctl/ioctl_rtc01.c
> > new file mode 100644
> > index 000000000..e097f8f65
> > --- /dev/null
> > +++ b/testcases/kernel/syscalls/ioctl/ioctl_rtc01.c
> > @@ -0,0 +1,121 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (c) 2020 Filip Bozuta Filip.Bozuta@rt-rk.com
> > + */
> > +
> > +/*
> > + * Test RTC ioctls with RTC_RD_TIME and RTC_SET_TIME requests
> > + *
> > + * Reads the current time from RTC device using RTC_RD_TIME
> > + * request and displays the time information as follows:
> > + * hour:minute, month day.month.year
> > + *
> > + * Sets a new time in RTC device using RTC_SET_TIME request
> > + * and displays the new time information as follws:
> > + * hour:minute, month day.month.year
> > + *
> > + * Reads the new time from RTC device using RTC_RD_TIME
> > + * request and checks whether the read time information
> > + * is same as the one set by RTC_SET_TIME
> > +
> > + * Runs RTC_SET_TIME to set back the current time read by
> > + * RTC_RD_TIME at the beginning of the test
> > + */
> > +
> > +#include <stdint.h>
> > +#include <errno.h>
> > +#include <linux/rtc.h>
> > +#include "tst_test.h"
> > +
> > +static void setup(void)
> > +{
> > + int exists = access("/dev/rtc", O_RDONLY);
> > +
> > + if (exists < 0)
> > + tst_brk(TCONF, "RTC device driver file not found");
>
> The old LTP test allowed an RTC device to be passed to the test, I
> wonder what should we do here. Should we loop over all rtcX devices under
> /dev/ by default?
>
Thank you for your review!
I will see to it that I correct all of the mistakes you have pointed
out to me in the v3 series of these patches.
I agree with this. I shall create a mechanism to run the test for all
/dev/rtcX devices that are available on the system.
> Also the same interface seems to be exported via proc and sysfs, it may make
> sense to check that /sys/class/rtc/rtcX/time and date matches the
> results from ioctl commands. We do have SAFE_FILE_SCANF() so reading
> these should be simple enough.
>
Yes you are right, this would be a good addition to the test. Should I
read the time using both the ioctl command and SAFE_FILE_SCANF() or
should I use just one of these two?
> > +}
> > +
> > +char *read_time_request = "RTC_RD_TIME";
> > +char *set_time_request = "RTC_SET_TIME";
> > +
> > +static void run(void)
> > +{
> > + int fd;
> > +
> > + struct rtc_time rtc_read_time;
> > + struct rtc_time rtc_cur_time;
> > + struct rtc_time rtc_set_time = {
> > + .tm_sec = 0, .tm_min = 15, .tm_hour = 13,
> > + .tm_mday = 26, .tm_mon = 8, .tm_year = 119};
> > +
> > + int time_read_supported, time_set_supported = 0;
> > +
> > + fd = SAFE_OPEN("/dev/rtc", O_RDONLY);
> > +
> > + if (fd == -1)
> > + tst_brk(TCONF, "RTC device driver file could not be opened");
>
> You are using SAFE_OPEN() the return value will never be -1, SAFE_OPEN()
> either succeeds or exits the test.
>
> You have to use raw open() call if you want to handle the errors
> yourself. Also you should really check for errno here, I guess that we
> should TCONF only or ENOENT?
>
> And lastly but not least we should open the device once in the test
> setup and close it once in test cleanup, there is no point in opening it
> for each test iteration (the test -i parameter).
>
Thank you for pointing this out to me. I will consider both options:
using SAFE_OPEN() and remove the error check or use raw open() with
appropriate error handling. In any case, I will move this opening of
the device file in the setup() function
and create a cleanup() function where the file will be closed.
> > + if (ioctl(fd, RTC_RD_TIME, &rtc_cur_time) == -1) {
>
> Just for the sake of being pedantic we should test that the retuned
> value here is exactly the one that describes success, so we should do:
>
> ioctl(fd, RTC_RD_TIME, &foo) != 0
>
> There have been cases where wrongly applied patch caused random values
> to be returned from a syscall which would not be caught here.
>
I will change this in every part of the test where the ioctl() call is checked.
> > + if (errno == ENOTTY)
> > + tst_res(TCONF, "ioctl %s not supported on RTC device",
> > + read_time_request);
> > + else
> > + tst_res(TFAIL | TERRNO, "unexpected ioctl error");
> > + } else {
> > + tst_res(TPASS, "time successfully read from RTC device");
> > + tst_res(TINFO, "current RTC time: %d:%d, %d.%d.%d",
> > + rtc_cur_time.tm_hour, rtc_cur_time.tm_min,
> > + rtc_cur_time.tm_mday, rtc_cur_time.tm_mon,
> > + rtc_cur_time.tm_year);
> > + time_read_supported = 1;
> > + }
> > +
> > + if (ioctl(fd, RTC_SET_TIME, &rtc_set_time) == -1) {
> > + if (errno == ENOTTY)
> > + tst_res(TCONF, "ioctl %s not supported on RTC device",
> > + set_time_request);
>
> Huh, why do we pass static string as %s? Why can't we just simply put it
> verbatim into the string?
>
> > + else
> > + tst_res(TFAIL | TERRNO, "unexpected ioctl error");
> > + } else {
> > + tst_res(TPASS, "time successfully set to RTC device");
> > + tst_res(TINFO, "new RTC time: %d:%d, %d.%d.%d",
> > + rtc_set_time.tm_hour, rtc_set_time.tm_min,
> > + rtc_set_time.tm_mday, rtc_set_time.tm_mon,
> > + rtc_set_time.tm_year);
> > + time_set_supported = 1;
> > + }
> > +
> > + if (time_read_supported && time_set_supported) {
> > + ioctl(fd, RTC_RD_TIME, &rtc_read_time);
> > +
> > + char time_data[][10] = {"minute", "hour", "month",
> > + "month day", "year"};
>
> This can be just const char *time_data[] = {...};
>
> > + int read_time_data[] = {
> > + rtc_read_time.tm_min, rtc_read_time.tm_hour,
> > + rtc_read_time.tm_mday, rtc_read_time.tm_mon,
> > + rtc_read_time.tm_year};
> > + int set_time_data[] = {
> > + rtc_set_time.tm_min, rtc_set_time.tm_hour,
> > + rtc_set_time.tm_mday, rtc_set_time.tm_mon,
> > + rtc_set_time.tm_year};
> > + for (int i = 0; i < 5; i++)
> > + if (read_time_data[i] == set_time_data[i])
> > + tst_res(TPASS, "%s reads new %s as expected",
> > + read_time_request, time_data[i]);
> > + else
> > + tst_res(TFAIL, "%s reads different %s than set",
> > + read_time_request, time_data[i]);
>
> Here as well, why do we pass static string as a parameter?
>
I am sorry. I got confused when writting this part of the test (last
20 lines). I will
change this whole time comparing section in v3 and remove all static
strings.
> > + }
> > +
> > + if (time_set_supported)
> > + ioctl(fd, RTC_SET_TIME, &rtc_cur_time);
>
> If we are allowed to set the time but not read it we will not restore
> the value at the end. Also does having write-only RTC even make sense?
> Shouldn't we just drop the time_read_supported flag and exit the test if
> we can't read it?
>
I will drop both time_read_supported and time_set_supported flags as
they are unnecessary. Instead, I shall just use tst_brk() to exit the
test in the beginning part if RTC_RD_TIME or RTC_SET_TIME request
fails (as there is no need to move forward with the test if any of
these requests fail at the beginning).
> > + SAFE_CLOSE(fd);
> > +}
> > +
> > +static struct tst_test test = {
> > + .test_all = run,
> > + .needs_root = 1,
> > + .needs_device = 1,
> > + .setup = setup,
> > +};
>
> Lastly there are some minor coding style violations, LTP uses LKML
> coding style, and there is a script packed along with the Linux kernel
> that can check your patches before sending them, see
> linux/scripts/checkpatch.pl.
>
I will check all future patches with checkpatch.pl script from the Linux kernel.
I would just like to inform you that I am going on a short holiday in
the next three days after which I will have some other tasks that I
have to attend to. For that reason, I won't be able to complete the v3
series with the corrected flaws of this patch in the near future.
However, I am looking forward to getting back at work on these tests
and correcting all of the mistakes that you have pointed out to me as
soon as possible. I am also looking forward to your reviews of my
future patches.
Thanks again for all of your comments!
Best regards,
Filip
> --
> Cyril Hrubis
> chrubis@suse.cz
More information about the ltp
mailing list