From: <Tim.Bird@sony.com>
To: <yzaikin@google.com>
Cc: <linux-kselftest@vger.kernel.org>, <linux-ext4@vger.kernel.org>,
<skhan@linuxfoundation.org>, <tytso@mit.edu>,
<adilger.kernel@dilger.ca>, <kunit-dev@googlegroups.com>,
<brendanhiggins@google.com>
Subject: RE: [PATCH linux-kselftest/test v2] ext4: add kunit test for decoding extended timestamps
Date: Thu, 10 Oct 2019 20:29:32 +0000 [thread overview]
Message-ID: <ECADFF3FD767C149AD96A924E7EA6EAF977C172D@USCULXMSG01.am.sony.com> (raw)
In-Reply-To: <CAAXuY3rk8_Eu_09jGbE66irtnjFzz4=6RxmZ6eMpBLdqpo5xfw@mail.gmail.com>
> -----Original Message-----
> From: Iurii Zaikin on Thursday, October 10, 2019 6:45 AM
>
> On Wed, Oct 9, 2019 at 8:47 PM <Tim.Bird@sony.com> wrote:
> >
> >
> > > -----Original Message-----
> > > From Iurii Zaikin on Wednesday, October 09, 2019 4:40 PM
> > >
> > > KUnit tests for decoding extended 64 bit timestamps.
> > >
> > > Signed-off-by: Iurii Zaikin <yzaikin@google.com>
> > > ---
> > > fs/ext4/Kconfig | 12 +++
> > > fs/ext4/Makefile | 1 +
> > > fs/ext4/inode-test.c | 221
> > > +++++++++++++++++++++++++++++++++++++++++++
> > > 3 files changed, 234 insertions(+)
> > > create mode 100644 fs/ext4/inode-test.c
> > >
> > > diff --git a/fs/ext4/Kconfig b/fs/ext4/Kconfig
> > > index cbb5ca830e57..cb0b52753674 100644
> > > --- a/fs/ext4/Kconfig
> > > +++ b/fs/ext4/Kconfig
> > > @@ -106,3 +106,15 @@ config EXT4_DEBUG
> > > If you select Y here, then you will be able to turn on debugging
> > > with a command such as:
> > > echo 1 > /sys/module/ext4/parameters/mballoc_debug
> > > +
> > > +config EXT4_KUNIT_TESTS
> > > + bool "KUnit test for ext4 inode"
> > > + depends on EXT4_FS
> > > + depends on KUNIT
> > > + help
> > > + This builds the ext4 inode sysctl unit test, which runs on boot.
> > > + Tests the encoding correctness of ext4 inode.
> > > + For more information on KUnit and unit tests in general please refer
> > > + to the KUnit documentation in Documentation/dev-tools/kunit/.
> > > +
> > > + If unsure, say N.
> > > diff --git a/fs/ext4/Makefile b/fs/ext4/Makefile
> > > index b17ddc229ac5..a0588fd2eea6 100644
> > > --- a/fs/ext4/Makefile
> > > +++ b/fs/ext4/Makefile
> > > @@ -13,4 +13,5 @@ ext4-y := balloc.o bitmap.o block_validity.o dir.o
> > > ext4_jbd2.o extents.o \
> > >
> > > ext4-$(CONFIG_EXT4_FS_POSIX_ACL) += acl.o
> > > ext4-$(CONFIG_EXT4_FS_SECURITY) += xattr_security.o
> > > +ext4-$(CONFIG_EXT4_KUNIT_TESTS) += inode-test.o
> > > ext4-$(CONFIG_FS_VERITY) += verity.o
> > > diff --git a/fs/ext4/inode-test.c b/fs/ext4/inode-test.c
> > > new file mode 100644
> > > index 000000000000..43bc6cb547cd
> > > --- /dev/null
> > > +++ b/fs/ext4/inode-test.c
> > > @@ -0,0 +1,221 @@
> > > +// 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.
> > > + * These tests are derived from the table under
> > > + * Documentation/filesystems/ext4/inodes.rst Inode Timestamps
> > > + */
> > > +
> > > +#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;
> > > +}
> > > +
> > > +
> > > +static void inode_test_xtimestamp_decoding(struct kunit *test)
> > > +{
> > > + const struct timestamp_expectation test_data[] = {
> > > + {
> > > + .test_case_name = "1901-12-13",
> > > + .msb_set = true,
> > > + .lower_bound = true,
> > > + .extra_bits = 0,
> > > + .expected = {.tv_sec = -0x80000000LL, .tv_nsec = 0L},
> > > + },
> > > +
> > > + {
> > > + .test_case_name = "1969-12-31",
> > > + .msb_set = true,
> > > + .lower_bound = false,
> > > + .extra_bits = 0,
> > > + .expected = {.tv_sec = -1LL, .tv_nsec = 0L},
> > > + },
> > > +
> > > + {
> > > + .test_case_name = "1970-01-01",
> > > + .msb_set = false,
> > > + .lower_bound = true,
> > > + .extra_bits = 0,
> > > + .expected = {0LL, 0L},
> > > + },
> > > +
> > > + {
> > > + .test_case_name = "2038-01-19",
> > > + .msb_set = false,
> > > + .lower_bound = false,
> > > + .extra_bits = 0,
> > > + .expected = {.tv_sec = 0x7fffffffLL, .tv_nsec = 0L},
> > > + },
> > > +
> > > + {
> > > + .test_case_name = "2038-01-19",
> > It's quite handy if testcase names can be unique, and describe what it is
> they are testing.
> >
> > If someone unfamiliar with this test looks at the results, it's nice if they can
> > intuit what it was that went wrong, based on the test case name.
> >
> > IMHO these names are too short and not descriptive enough.
>
> The test cases are pretty much 1:1 to the examples table referenced at
> the top comment of the file. Would it help if I move the reference
> comment closer to the test case definition or would you like the test
> name to have a reference to a table entry encoded into it?
I think moving the comment to right above the testcase definitions
would be good. Somehow I missed that.
OK - I also missed the usage of the TESTCASE_NAME_FORMAT string. This obviously
handles the issue of the testcase names being unique, but doesn't help those not
familiar with the test.
What I'm suggesting is just a little bit of extra wording, describing in English
what the test is checking for. This is for people looking
at test results who don't know the internals of the test.
I'm pretty sure these are the wrong descriptions, but something like this:
{
.test_case_name = "2038-01-19 check upper edge of 31-bit boundary",
.msb_set = false,
.lower_bound = false,
.extra_bits = 0,
.expected = {.tv_sec = 0x7fffffffLL, .tv_nsec = 0L},
},
{
.test_case_name = "2038-01-19 check first use of extra epoch bit",
.msb_set = true,
.lower_bound = true,
.extra_bits = 1,
.expected = {.tv_sec = 0x80000000LL, .tv_nsec = 0L},
},
I'm not pedantic about it.
-- Tim
> > > + .msb_set = true,
> > > + .lower_bound = true,
> > > + .extra_bits = 1,
> > > + .expected = {.tv_sec = 0x80000000LL, .tv_nsec = 0L},
> > > + },
> > > + .msb_set = true,
> > > + .lower_bound = true,
> > > + .extra_bits = 1,
> > > + .expected = {.tv_sec = 0x80000000LL, .tv_nsec = 0L},
> > > + },
> > > +
> > > + {
> > > + .test_case_name = "2106-02-07",
> > > + .msb_set = true,
> > > + .lower_bound = false,
> > > + .extra_bits = 1,
> > > + .expected = {.tv_sec = 0xffffffffLL, .tv_nsec = 0L},
> > > + },
> > > +
> > > + {
> > > + .test_case_name = "2106-02-07",
> > > + .msb_set = false,
> > > + .lower_bound = true,
> > > + .extra_bits = 1,
> > > + .expected = {.tv_sec = 0x100000000LL, .tv_nsec = 0L},
> > > + },
> > > +
> > > + {
> > > + .test_case_name = "2174-02-25",
> > > + .msb_set = false,
> > > + .lower_bound = false,
> > > + .extra_bits = 1,
> > > + .expected = {.tv_sec = 0x17fffffffLL, .tv_nsec = 0L},
> > > + },
> > > +
> > > + {
> > > + .test_case_name = "2174-02-25",
> > > + .msb_set = true,
> > > + .lower_bound = true,
> > > + .extra_bits = 2,
> > > + .expected = {.tv_sec = 0x180000000LL, .tv_nsec = 0L},
> > > + },
> > > +
> > > + {
> > > + .test_case_name = "2242-03-16",
> > > + .msb_set = true,
> > > + .lower_bound = false,
> > > + .extra_bits = 2,
> > > + .expected = {.tv_sec = 0x1ffffffffLL, .tv_nsec = 0L},
> > > + },
> > > +
> > > + {
> > > + .test_case_name = "2242-03-16",
> > > + .msb_set = false,
> > > + .lower_bound = true,
> > > + .extra_bits = 2,
> > > + .expected = {.tv_sec = 0x200000000LL, .tv_nsec = 0L},
> > > + },
> > > +
> > > + {
> > > + .test_case_name = " 2310-04-04",
> > > + .msb_set = false,
> > > + .lower_bound = false,
> > > + .extra_bits = 2,
> > > + .expected = {.tv_sec = 0x27fffffffLL, .tv_nsec = 0L},
> > > + },
> > > +
> > > + {
> > > + .test_case_name = " 2310-04-04 00:00:00.1",
> > > + .msb_set = false,
> > > + .lower_bound = false,
> > > + .extra_bits = 6,
> > > + .expected = {.tv_sec = 0x27fffffffLL, .tv_nsec = 1L},
> > > + },
> > > +
> > > + {
> > > + .test_case_name = "2378-04-22
> > > 00:00:00.MAX_NSEC",
> > > + .msb_set = false,
> > > + .lower_bound = true,
> > > + .extra_bits = 0xFFFFFFFF,
> > > + .expected = {.tv_sec = 0x300000000LL,
> > > + .tv_nsec = MAX_NANOSECONDS},
> > > + },
> > > +
> > > + {
> > > + .test_case_name = "2378-04-22",
> > > + .msb_set = false,
> > > + .lower_bound = true,
> > > + .extra_bits = 3,
> > > + .expected = {.tv_sec = 0x300000000LL, .tv_nsec = 0L},
> > > + },
> > > +
> > > + {
> > > + .test_case_name = "2446-05-10",
> > > + .msb_set = false,
> > > + .lower_bound = false,
> > > + .extra_bits = 3,
> > > + .expected = {.tv_sec = 0x37fffffffLL, .tv_nsec = 0L},
> > > + }
> > > + };
> > > +
> > > + struct timespec64 timestamp;
> > > + int i;
> > > +
> > > + for (i = 0; i < ARRAY_SIZE(test_data); ++i) {
> > > + timestamp.tv_sec = get_32bit_time(&test_data[i]);
> > > + ext4_decode_extra_time(×tamp,
> > > + cpu_to_le32(test_data[i].extra_bits));
> > > +
> > > + KUNIT_EXPECT_EQ_MSG(test,
> > > + test_data[i].expected.tv_sec,
> > > + timestamp.tv_sec,
> > > + CASE_NAME_FORMAT,
> > > + test_data[i].test_case_name,
> > > + test_data[i].msb_set,
> > > + test_data[i].lower_bound,
> > > + test_data[i].extra_bits);
> > > + KUNIT_EXPECT_EQ_MSG(test,
> > > + test_data[i].expected.tv_nsec,
> > > + timestamp.tv_nsec,
> > > + CASE_NAME_FORMAT,
> > > + test_data[i].test_case_name,
> > > + test_data[i].msb_set,
> > > + test_data[i].lower_bound,
> > > + test_data[i].extra_bits);
> > > + }
> > > +}
> > > +
> > > +static struct kunit_case ext4_inode_test_cases[] = {
> > > + KUNIT_CASE(inode_test_xtimestamp_decoding),
> > > + {}
> > > +};
> > > +
> > > +static struct kunit_suite ext4_inode_test_suite = {
> > > + .name = "ext4_inode_test",
> > > + .test_cases = ext4_inode_test_cases,
> > > +};
> > > +
> > > +kunit_test_suite(ext4_inode_test_suite);
> > > --
> > > 2.23.0.700.g56cf767bdb-goog
next prev parent reply other threads:[~2019-10-10 20:30 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-10 2:39 [PATCH linux-kselftest/test v2] ext4: add kunit test for decoding extended timestamps Iurii Zaikin
2019-10-10 3:46 ` Tim.Bird
2019-10-10 16:45 ` Iurii Zaikin
2019-10-10 20:29 ` Tim.Bird [this message]
2019-10-10 23:49 ` Iurii Zaikin
2019-10-10 17:11 ` Shuah Khan
2019-10-10 22:13 ` Iurii Zaikin
2019-10-11 10:05 ` Brendan Higgins
2019-10-11 13:19 ` Theodore Y. Ts'o
2019-10-12 2:38 ` Iurii Zaikin
2019-10-16 22:18 ` Brendan Higgins
2019-10-16 23:26 ` Shuah Khan
2019-10-17 0:07 ` Iurii Zaikin
2019-10-17 12:08 ` Theodore Y. Ts'o
2019-10-17 22:25 ` Tim.Bird
2019-10-17 22:56 ` Theodore Y. Ts'o
2019-10-17 23:40 ` Tim.Bird
2019-10-18 1:40 ` Theodore Y. Ts'o
2019-10-18 2:40 ` Tim.Bird
2019-10-18 15:27 ` Theodore Y. Ts'o
2019-10-18 20:24 ` Shuah Khan
2019-10-24 1:30 ` Brendan Higgins
2019-10-18 1:12 ` Brendan Higgins
2019-10-18 1:30 ` Tim.Bird
2019-10-17 22:49 ` Shuah Khan
2019-10-17 23:07 ` Iurii Zaikin
2019-10-17 23:12 ` Shuah Khan
2019-10-17 23:27 ` Iurii Zaikin
2019-10-17 23:42 ` Shuah Khan
2019-10-17 23:54 ` Tim.Bird
2019-10-17 23:59 ` Shuah Khan
2019-10-18 0:11 ` Iurii Zaikin
2019-10-18 0:38 ` Tim.Bird
2019-10-18 1:06 ` Iurii Zaikin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ECADFF3FD767C149AD96A924E7EA6EAF977C172D@USCULXMSG01.am.sony.com \
--to=tim.bird@sony.com \
--cc=adilger.kernel@dilger.ca \
--cc=brendanhiggins@google.com \
--cc=kunit-dev@googlegroups.com \
--cc=linux-ext4@vger.kernel.org \
--cc=linux-kselftest@vger.kernel.org \
--cc=skhan@linuxfoundation.org \
--cc=tytso@mit.edu \
--cc=yzaikin@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).