Re: [PATCH linux-kselftest/test v4] ext4: add kunit test for decoding extended timestamps

[Iurii Zaikin <yzaikin@google.com>]

On Wed, Oct 16, 2019 at 1:03 PM Brendan Higgins
<brendanhiggins@google.com> wrote:
>
> On Fri, Oct 11, 2019 at 07:37:57PM -0700, Iurii Zaikin wrote:
> > KUnit tests for decoding extended 64 bit timestamps
> > that verify the seconds part of [a/c/m]
> > timestamps in ext4 inode structs are decoded correctly.
> > KUnit tests, which run on boot and output
> > the results to the debug log in TAP format (http://testanything.org/).
> > are only useful for kernel devs running KUnit test harness. Not for
> > inclusion into a production build.
> > Test data is derive from the table under
> nit:                ^
> Should be:     derived from ...
>
Oops. Done.
> > Documentation/filesystems/ext4/inodes.rst Inode Timestamps.
> >
> > Signed-off-by: Iurii Zaikin <yzaikin@google.com>
>
> Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
> Tested-by: Brendan Higgins <brendanhiggins@google.com>
>
> > ---
> >  fs/ext4/Kconfig      |  14 +++
> >  fs/ext4/Makefile     |   1 +
> >  fs/ext4/inode-test.c | 239 +++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 254 insertions(+)
> >  create mode 100644 fs/ext4/inode-test.c
> >
> [...]
> > diff --git a/fs/ext4/inode-test.c b/fs/ext4/inode-test.c
> > new file mode 100644
> > index 000000000000..3b3a453ff382
> > --- /dev/null
> > +++ b/fs/ext4/inode-test.c
> > @@ -0,0 +1,239 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * KUnit test of ext4 inode that verify the seconds part of [a/c/m]
> > + * timestamps in ext4 inode structs are decoded correctly.
> > + */
> > +
> > +#include <kunit/test.h>
> > +#include <linux/kernel.h>
> > +#include <linux/time64.h>
> > +
> > +#include "ext4.h"
> > +
> > +/* binary: 00000000 00000000 00000000 00000000 */
> > +#define LOWER_MSB_0 0L
> > +/* binary: 01111111 11111111 11111111 11111111 */
> > +#define UPPER_MSB_0 0x7fffffffL
> > +/* binary: 10000000 00000000 00000000 00000000 */
> > +#define LOWER_MSB_1 (-0x80000000L)
> > +/* binary: 11111111 11111111 11111111 11111111 */
> > +#define UPPER_MSB_1 (-1L)
> > +/* binary: 00111111 11111111 11111111 11111111 */
> > +#define MAX_NANOSECONDS ((1L << 30) - 1)
> > +
> > +#define CASE_NAME_FORMAT "%s: msb:%x lower_bound:%x extra_bits: %x"
> > +
> > +struct timestamp_expectation {
> > +     const char *test_case_name;
> > +     struct timespec64 expected;
> > +     u32 extra_bits;
> > +     bool msb_set;
> > +     bool lower_bound;
> > +};
> > +
> > +static time64_t get_32bit_time(const struct timestamp_expectation * const test)
> > +{
> > +     if (test->msb_set) {
> > +             if (test->lower_bound)
> > +                     return LOWER_MSB_1;
> > +
> > +             return UPPER_MSB_1;
> > +     }
> > +
> > +     if (test->lower_bound)
> > +             return LOWER_MSB_0;
> > +     return UPPER_MSB_0;
> > +}
> > +
> > +
> > +/*
> > + * These tests are derived from the table under
> > + * Documentation/filesystems/ext4/inodes.rst Inode Timestamps
> > + */
> > +static void inode_test_xtimestamp_decoding(struct kunit *test)
> > +{
> > +     const struct timestamp_expectation test_data[] = {
> > +             {
> > +                     .test_case_name =
> > +             "1901-12-13 Lower bound of 32bit < 0 timestamp, no extra bits.",
>
> nit: Maybe drop the period at the end (here and elsewhere)? Otherwise if
> the test fails you have a period right next to a colon and it looks a
> bit off.
>
Done
> > +                     .msb_set = true,
> > +                     .lower_bound = true,
> > +                     .extra_bits = 0,
> > +                     .expected = {.tv_sec = -0x80000000LL, .tv_nsec = 0L},
> > +             },
>
> Feel free to ignore my nits if you don't need to send another version.
> Also note that Ted has given a Reviewed-by on an earlier revision.
>
> Thanks!