[PATCH v8] fat: Add KUnit tests for checksums and timestamps

[PATCH v8] fat: Add KUnit tests for checksums and timestamps

[David Gow]

Add some basic sanity-check tests for the fat_checksum() function and
the fat_time_unix2fat() and fat_time_fat2unix() functions. These unit
tests verify these functions return correct output for a number of test
inputs.

These tests were inspored by -- and serve a similar purpose to -- the
timestamp parsing KUnit tests in ext4[1].

Note that, unlike fat_time_unix2fat, fat_time_fat2unix wasn't previously
exported, so this patch exports it as well. This is required for the
case where we're building the fat and fat_test as modules.

[1]:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/ext4/inode-test.c

Signed-off-by: David Gow <davidgow@google.com>
Acked-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
---

It's been a while, but this hopefully is a final version of the FAT KUnit
patchset. It has a number of changes to keep it up-to-date with current
KUnit standards, notably the use of parameterised tests and the addition
of a '.kunitconfig' file to allow for easy testing. It also fixes an
endianness tagging issue picked up by the kernel test robot under sparse
on pa-risc.

Cheers,
-- David

Changes since v7:
https://lore.kernel.org/linux-kselftest/20201028064631.3774908-1-davidgow@google.com/
- Make the two timestamp tests parameterised: this means that the KUnit
  runtime and tooling are aware of the different testcases (and print a
  nice list of them to the TAP log when the test is run).
- Fix some issues sparse picked up with __le32 tagged integers.
- Add an fs/fat/.kunitconfig file which contains all the Kconfig entries
  needed to run the test. The test can now be run with:
  ./tools/testing/kunit/kunit.py run --kunitconfig fs/fat/.kunitconfig

Changes since v6:
https://lore.kernel.org/linux-kselftest/20201024060558.2556249-1-davidgow@google.com/
- Make CONFIG_FAT_DEFAULT_CODEPAGE depend on FAT_FS, rather than either
  VFAT_FS or MSDOS_FS.
  - This means that FAT_KUNIT_TEST can now also just depend of FAT_FS
- Fix a few warnings that KUnit tool was eating:
  - KUnit's type checking needs a specific cast for the fat_checksum()
    expected results.
  - The time test cases shouldn't be 'const'
  - The fake superblock is now static, as otherwise it increased the
    stack size too much.

Changes since v4/5:
https://lore.kernel.org/linux-kselftest/20201024052047.2526780-1-davidgow@google.com/
- Fix a typo introduced in the Kconfig. It builds now.

Changes since v3:
https://lore.kernel.org/linux-kselftest/20201021061713.1545931-1-davidgow@google.com/
- Update the Kconfig entry to use "depends on" rather than "select", as
  discussed in [2].
- Depend on "MSDOS_FS || VFAT_FS", rather than "FAT_FS", as we need the
  CONFIG_FAT_DEFAULT_CODEPAGE symbol to be defined.

Changes since v2:
https://lore.kernel.org/linux-kselftest/20201020055856.1270482-1-davidgow@google.com/
- Comment that the export for fat_time_fat2unix() function is for KUnit
  tests.

Changes since v1:
https://lore.kernel.org/linux-kselftest/20201017064107.375174-1-davidgow@google.com/
- Now export fat_time_fat2unix() so that the test can access it when
  built as a module.


[2]:
https://lore.kernel.org/linux-ext4/52959e99-4105-3de9-730c-c46894b82bdd@infradead.org/T/#t



 fs/fat/.kunitconfig |   5 ++
 fs/fat/Kconfig      |  14 +++-
 fs/fat/Makefile     |   2 +
 fs/fat/fat_test.c   | 197 ++++++++++++++++++++++++++++++++++++++++++++
 fs/fat/misc.c       |   2 +
 5 files changed, 219 insertions(+), 1 deletion(-)
 create mode 100644 fs/fat/.kunitconfig
 create mode 100644 fs/fat/fat_test.c

diff --git a/fs/fat/.kunitconfig b/fs/fat/.kunitconfig
new file mode 100644
index 000000000000..0a6971dbeccb
--- /dev/null
+++ b/fs/fat/.kunitconfig
@@ -0,0 +1,5 @@
+CONFIG_KUNIT=y
+CONFIG_FAT_FS=y
+CONFIG_MSDOS_FS=y
+CONFIG_VFAT_FS=y
+CONFIG_FAT_KUNIT_TEST=y
diff --git a/fs/fat/Kconfig b/fs/fat/Kconfig
index 66532a71e8fd..238cc55f84c4 100644
--- a/fs/fat/Kconfig
+++ b/fs/fat/Kconfig
@@ -77,7 +77,7 @@ config VFAT_FS
 
 config FAT_DEFAULT_CODEPAGE
 	int "Default codepage for FAT"
-	depends on MSDOS_FS || VFAT_FS
+	depends on FAT_FS
 	default 437
 	help
 	  This option should be set to the codepage of your FAT filesystems.
@@ -115,3 +115,15 @@ config FAT_DEFAULT_UTF8
 	  Say Y if you use UTF-8 encoding for file names, N otherwise.
 
 	  See <file:Documentation/filesystems/vfat.rst> for more information.
+
+config FAT_KUNIT_TEST
+	tristate "Unit Tests for FAT filesystems" if !KUNIT_ALL_TESTS
+	depends on KUNIT && FAT_FS
+	default KUNIT_ALL_TESTS
+	help
+	  This builds the FAT KUnit tests
+
+	  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/fat/Makefile b/fs/fat/Makefile
index 70645ce2f7fc..2b034112690d 100644
--- a/fs/fat/Makefile
+++ b/fs/fat/Makefile
@@ -10,3 +10,5 @@ obj-$(CONFIG_MSDOS_FS) += msdos.o
 fat-y := cache.o dir.o fatent.o file.o inode.o misc.o nfs.o
 vfat-y := namei_vfat.o
 msdos-y := namei_msdos.o
+
+obj-$(CONFIG_FAT_KUNIT_TEST) += fat_test.o
diff --git a/fs/fat/fat_test.c b/fs/fat/fat_test.c
new file mode 100644
index 000000000000..febd25f57d4b
--- /dev/null
+++ b/fs/fat/fat_test.c
@@ -0,0 +1,197 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * KUnit tests for FAT filesystems.
+ *
+ * Copyright (C) 2020 Google LLC.
+ * Author: David Gow <davidgow@google.com>
+ */
+
+#include <kunit/test.h>
+
+#include "fat.h"
+
+static void fat_checksum_test(struct kunit *test)
+{
+	/* With no extension. */
+	KUNIT_EXPECT_EQ(test, fat_checksum("VMLINUX    "), (u8)44);
+	/* With 3-letter extension. */
+	KUNIT_EXPECT_EQ(test, fat_checksum("README  TXT"), (u8)115);
+	/* With short (1-letter) extension. */
+	KUNIT_EXPECT_EQ(test, fat_checksum("ABCDEFGHA  "), (u8)98);
+}
+
+
+struct fat_timestamp_testcase {
+	const char *name;
+	struct timespec64 ts;
+	__le16 time;
+	__le16 date;
+	u8 cs;
+	int time_offset;
+};
+
+static struct fat_timestamp_testcase time_test_cases[] = {
+	{
+		.name = "Earliest possible UTC (1980-01-01 00:00:00)",
+		.ts = {.tv_sec = 315532800LL, .tv_nsec = 0L},
+		.time = cpu_to_le16(0),
+		.date = cpu_to_le16(33),
+		.cs = 0,
+		.time_offset = 0,
+	},
+	{
+		.name = "Latest possible UTC (2107-12-31 23:59:58)",
+		.ts = {.tv_sec = 4354819198LL, .tv_nsec = 0L},
+		.time = cpu_to_le16(49021),
+		.date = cpu_to_le16(65439),
+		.cs = 0,
+		.time_offset = 0,
+	},
+	{
+		.name = "Earliest possible (UTC-11) (== 1979-12-31 13:00:00 UTC)",
+		.ts = {.tv_sec = 315493200LL, .tv_nsec = 0L},
+		.time = cpu_to_le16(0),
+		.date = cpu_to_le16(33),
+		.cs = 0,
+		.time_offset = 11 * 60,
+	},
+	{
+		.name = "Latest possible (UTC+11) (== 2108-01-01 10:59:58 UTC)",
+		.ts = {.tv_sec = 4354858798LL, .tv_nsec = 0L},
+		.time = cpu_to_le16(49021),
+		.date = cpu_to_le16(65439),
+		.cs = 0,
+		.time_offset = -11 * 60,
+	},
+	{
+		.name = "Leap Day / Year (1996-02-29 00:00:00)",
+		.ts = {.tv_sec = 825552000LL, .tv_nsec = 0L},
+		.time = cpu_to_le16(0),
+		.date = cpu_to_le16(8285),
+		.cs = 0,
+		.time_offset = 0,
+	},
+	{
+		.name = "Year 2000 is leap year (2000-02-29 00:00:00)",
+		.ts = {.tv_sec = 951782400LL, .tv_nsec = 0L},
+		.time = cpu_to_le16(0),
+		.date = cpu_to_le16(10333),
+		.cs = 0,
+		.time_offset = 0,
+	},
+	{
+		.name = "Year 2100 not leap year (2100-03-01 00:00:00)",
+		.ts = {.tv_sec = 4107542400LL, .tv_nsec = 0L},
+		.time = cpu_to_le16(0),
+		.date = cpu_to_le16(61537),
+		.cs = 0,
+		.time_offset = 0,
+	},
+	{
+		.name = "Leap year + timezone UTC+1 (== 2004-02-29 00:30:00 UTC)",
+		.ts = {.tv_sec = 1078014600LL, .tv_nsec = 0L},
+		.time = cpu_to_le16(48064),
+		.date = cpu_to_le16(12380),
+		.cs = 0,
+		.time_offset = -60,
+	},
+	{
+		.name = "Leap year + timezone UTC-1 (== 2004-02-29 23:30:00 UTC)",
+		.ts = {.tv_sec = 1078097400LL, .tv_nsec = 0L},
+		.time = cpu_to_le16(960),
+		.date = cpu_to_le16(12385),
+		.cs = 0,
+		.time_offset = 60,
+	},
+	{
+		.name = "VFAT odd-second resolution (1999-12-31 23:59:59)",
+		.ts = {.tv_sec = 946684799LL, .tv_nsec = 0L},
+		.time = cpu_to_le16(49021),
+		.date = cpu_to_le16(10143),
+		.cs = 100,
+		.time_offset = 0,
+	},
+	{
+		.name = "VFAT 10ms resolution (1980-01-01 00:00:00:0010)",
+		.ts = {.tv_sec = 315532800LL, .tv_nsec = 10000000L},
+		.time = cpu_to_le16(0),
+		.date = cpu_to_le16(33),
+		.cs = 1,
+		.time_offset = 0,
+	},
+};
+
+static void time_testcase_desc(struct fat_timestamp_testcase *t,
+			       char *desc)
+{
+	strscpy(desc, t->name, KUNIT_PARAM_DESC_SIZE);
+}
+
+KUNIT_ARRAY_PARAM(fat_time, time_test_cases, time_testcase_desc);
+
+static void fat_time_fat2unix_test(struct kunit *test)
+{
+	static struct msdos_sb_info fake_sb;
+	struct timespec64 ts;
+	struct fat_timestamp_testcase *testcase =
+		(struct fat_timestamp_testcase *)test->param_value;
+
+	fake_sb.options.tz_set = 1;
+	fake_sb.options.time_offset = testcase->time_offset;
+
+	fat_time_fat2unix(&fake_sb, &ts,
+			  testcase->time,
+			  testcase->date,
+			  testcase->cs);
+	KUNIT_EXPECT_EQ_MSG(test,
+			    testcase->ts.tv_sec,
+			    ts.tv_sec,
+			    "Timestamp mismatch (seconds)\n");
+	KUNIT_EXPECT_EQ_MSG(test,
+			    testcase->ts.tv_nsec,
+			    ts.tv_nsec,
+			    "Timestamp mismatch (nanoseconds)\n");
+}
+
+static void fat_time_unix2fat_test(struct kunit *test)
+{
+	static struct msdos_sb_info fake_sb;
+	__le16 date, time;
+	u8 cs;
+	struct fat_timestamp_testcase *testcase =
+		(struct fat_timestamp_testcase *)test->param_value;
+
+	fake_sb.options.tz_set = 1;
+	fake_sb.options.time_offset = testcase->time_offset;
+
+	fat_time_unix2fat(&fake_sb, &(testcase->ts),
+			  &time, &date, &cs);
+	KUNIT_EXPECT_EQ_MSG(test,
+			    le16_to_cpu(testcase->time),
+			    le16_to_cpu(time),
+			    "Time mismatch\n");
+	KUNIT_EXPECT_EQ_MSG(test,
+			    le16_to_cpu(testcase->date),
+			    le16_to_cpu(date),
+			    "Date mismatch\n");
+	KUNIT_EXPECT_EQ_MSG(test,
+			    testcase->cs,
+			    cs,
+			    "Centisecond mismatch\n");
+}
+
+static struct kunit_case fat_test_cases[] = {
+	KUNIT_CASE(fat_checksum_test),
+	KUNIT_CASE_PARAM(fat_time_fat2unix_test, fat_time_gen_params),
+	KUNIT_CASE_PARAM(fat_time_unix2fat_test, fat_time_gen_params),
+	{},
+};
+
+static struct kunit_suite fat_test_suite = {
+	.name = "fat_test",
+	.test_cases = fat_test_cases,
+};
+
+kunit_test_suites(&fat_test_suite);
+
+MODULE_LICENSE("GPL v2");
diff --git a/fs/fat/misc.c b/fs/fat/misc.c
index 18a50a46b57f..9073fa927be3 100644
--- a/fs/fat/misc.c
+++ b/fs/fat/misc.c
@@ -229,6 +229,8 @@ void fat_time_fat2unix(struct msdos_sb_info *sbi, struct timespec64 *ts,
 		ts->tv_nsec = 0;
 	}
 }
+/* Export fat_time_fat2unix() for the fat_test KUnit tests. */
+EXPORT_SYMBOL_GPL(fat_time_fat2unix);
 
 /* Convert linear UNIX date to a FAT time/date pair. */
 void fat_time_unix2fat(struct msdos_sb_info *sbi, struct timespec64 *ts,
-- 
2.31.1.368.gbe11c130af-goog

Re: [PATCH v8] fat: Add KUnit tests for checksums and timestamps

[Brendan Higgins]

On Thu, Apr 15, 2021 at 11:56:23PM -0700, David Gow wrote:
> Add some basic sanity-check tests for the fat_checksum() function and
> the fat_time_unix2fat() and fat_time_fat2unix() functions. These unit
> tests verify these functions return correct output for a number of test
> inputs.
> 
> These tests were inspored by -- and serve a similar purpose to -- the
                   ^^^^^^^^
        I am guessing this is supposed to be "inspired".

> timestamp parsing KUnit tests in ext4[1].
> 
> Note that, unlike fat_time_unix2fat, fat_time_fat2unix wasn't previously
> exported, so this patch exports it as well. This is required for the
> case where we're building the fat and fat_test as modules.
> 
> [1]:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/ext4/inode-test.c
> 
> Signed-off-by: David Gow <davidgow@google.com>
> Acked-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Aside from the nit above, and the *potential* nit and question below.
Everything here looks good to me.

Reviewed-by: Brendan Higgins <brendanhiggins@google.com>

> ---
> 
> It's been a while, but this hopefully is a final version of the FAT KUnit
> patchset. It has a number of changes to keep it up-to-date with current
> KUnit standards, notably the use of parameterised tests and the addition
> of a '.kunitconfig' file to allow for easy testing. It also fixes an
> endianness tagging issue picked up by the kernel test robot under sparse
> on pa-risc.
> 
> Cheers,
> -- David

[...]

> diff --git a/fs/fat/fat_test.c b/fs/fat/fat_test.c
> new file mode 100644
> index 000000000000..febd25f57d4b
> --- /dev/null
> +++ b/fs/fat/fat_test.c
> @@ -0,0 +1,197 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * KUnit tests for FAT filesystems.
> + *
> + * Copyright (C) 2020 Google LLC.

Nit: I know you wrote this last year, but I have had other maintainers
tell me the Copyright date should be set to when the final version of
the patch is sent out.

I personally don't care, and I don't think you should resend this patch
just for that, but figured I would mention.

> + * Author: David Gow <davidgow@google.com>
> + */
> +
> +#include <kunit/test.h>
> +
> +#include "fat.h"
> +
> +static void fat_checksum_test(struct kunit *test)
> +{
> +	/* With no extension. */
> +	KUNIT_EXPECT_EQ(test, fat_checksum("VMLINUX    "), (u8)44);
> +	/* With 3-letter extension. */
> +	KUNIT_EXPECT_EQ(test, fat_checksum("README  TXT"), (u8)115);
> +	/* With short (1-letter) extension. */
> +	KUNIT_EXPECT_EQ(test, fat_checksum("ABCDEFGHA  "), (u8)98);

How do you get the magic values? Or is this just supposed to be a
regression test?

Not going to pretend I understand FAT, but everything else in this test
makes sense from a logical/testing/readability point of view.

Cheers!

[...]

Re: [PATCH v8] fat: Add KUnit tests for checksums and timestamps

[David Gow]

On Wed, May 5, 2021 at 2:36 AM 'Brendan Higgins' via KUnit Development
<kunit-dev@googlegroups.com> wrote:
>
> On Thu, Apr 15, 2021 at 11:56:23PM -0700, David Gow wrote:
> > Add some basic sanity-check tests for the fat_checksum() function and
> > the fat_time_unix2fat() and fat_time_fat2unix() functions. These unit
> > tests verify these functions return correct output for a number of test
> > inputs.
> >
> > These tests were inspored by -- and serve a similar purpose to -- the
>                    ^^^^^^^^
>         I am guessing this is supposed to be "inspired".
>

Oops -- yup. This is a typo. I can resend a version with this fixed if
you think that makes sense, otherwise I'll just hold it over in case I
need to send out a new version.

> > timestamp parsing KUnit tests in ext4[1].
> >
> > Note that, unlike fat_time_unix2fat, fat_time_fat2unix wasn't previously
> > exported, so this patch exports it as well. This is required for the
> > case where we're building the fat and fat_test as modules.
> >
> > [1]:
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/ext4/inode-test.c
> >
> > Signed-off-by: David Gow <davidgow@google.com>
> > Acked-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
>
> Aside from the nit above, and the *potential* nit and question below.
> Everything here looks good to me.
>
> Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
>
> > ---
> >
> > It's been a while, but this hopefully is a final version of the FAT KUnit
> > patchset. It has a number of changes to keep it up-to-date with current
> > KUnit standards, notably the use of parameterised tests and the addition
> > of a '.kunitconfig' file to allow for easy testing. It also fixes an
> > endianness tagging issue picked up by the kernel test robot under sparse
> > on pa-risc.
> >
> > Cheers,
> > -- David
>
> [...]
>
> > diff --git a/fs/fat/fat_test.c b/fs/fat/fat_test.c
> > new file mode 100644
> > index 000000000000..febd25f57d4b
> > --- /dev/null
> > +++ b/fs/fat/fat_test.c
> > @@ -0,0 +1,197 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * KUnit tests for FAT filesystems.
> > + *
> > + * Copyright (C) 2020 Google LLC.
>
> Nit: I know you wrote this last year, but I have had other maintainers
> tell me the Copyright date should be set to when the final version of
> the patch is sent out.
>
> I personally don't care, and I don't think you should resend this patch
> just for that, but figured I would mention.
>

Hmm... I've definitely heard this both ways, but I can easily update
the year if I need to send a new version out.

> > + * Author: David Gow <davidgow@google.com>
> > + */
> > +
> > +#include <kunit/test.h>
> > +
> > +#include "fat.h"
> > +
> > +static void fat_checksum_test(struct kunit *test)
> > +{
> > +     /* With no extension. */
> > +     KUNIT_EXPECT_EQ(test, fat_checksum("VMLINUX    "), (u8)44);
> > +     /* With 3-letter extension. */
> > +     KUNIT_EXPECT_EQ(test, fat_checksum("README  TXT"), (u8)115);
> > +     /* With short (1-letter) extension. */
> > +     KUNIT_EXPECT_EQ(test, fat_checksum("ABCDEFGHA  "), (u8)98);
>
> How do you get the magic values? Or is this just supposed to be a
> regression test?

This is mainly meant to be a regression test, and the values did
originally come from just running fat_checksum. I have, however,
checked that Windows 98 produces the same values (on a FAT12
filesystem).

> Not going to pretend I understand FAT, but everything else in this test
> makes sense from a logical/testing/readability point of view.
>
> Cheers!
>
> [...]
>
> --
> You received this message because you are subscribed to the Google Groups "KUnit Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/YJGUFrc8PJ0LAKiF%40google.com.

Re: [PATCH v8] fat: Add KUnit tests for checksums and timestamps

[Brendan Higgins]

On Tue, May 4, 2021 at 11:48 PM David Gow <davidgow@google.com> wrote:
>
> On Wed, May 5, 2021 at 2:36 AM 'Brendan Higgins' via KUnit Development
> <kunit-dev@googlegroups.com> wrote:
> >
> > On Thu, Apr 15, 2021 at 11:56:23PM -0700, David Gow wrote:
> > > Add some basic sanity-check tests for the fat_checksum() function and
> > > the fat_time_unix2fat() and fat_time_fat2unix() functions. These unit
> > > tests verify these functions return correct output for a number of test
> > > inputs.
> > >
> > > These tests were inspored by -- and serve a similar purpose to -- the
> >                    ^^^^^^^^
> >         I am guessing this is supposed to be "inspired".
> >
>
> Oops -- yup. This is a typo. I can resend a version with this fixed if
> you think that makes sense, otherwise I'll just hold it over in case I
> need to send out a new version.
>
> > > timestamp parsing KUnit tests in ext4[1].
> > >
> > > Note that, unlike fat_time_unix2fat, fat_time_fat2unix wasn't previously
> > > exported, so this patch exports it as well. This is required for the
> > > case where we're building the fat and fat_test as modules.
> > >
> > > [1]:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/ext4/inode-test.c
> > >
> > > Signed-off-by: David Gow <davidgow@google.com>
> > > Acked-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
> >
> > Aside from the nit above, and the *potential* nit and question below.
> > Everything here looks good to me.
> >
> > Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
> >
> > > ---
> > >
> > > It's been a while, but this hopefully is a final version of the FAT KUnit
> > > patchset. It has a number of changes to keep it up-to-date with current
> > > KUnit standards, notably the use of parameterised tests and the addition
> > > of a '.kunitconfig' file to allow for easy testing. It also fixes an
> > > endianness tagging issue picked up by the kernel test robot under sparse
> > > on pa-risc.
> > >
> > > Cheers,
> > > -- David
> >
> > [...]
> >
> > > diff --git a/fs/fat/fat_test.c b/fs/fat/fat_test.c
> > > new file mode 100644
> > > index 000000000000..febd25f57d4b
> > > --- /dev/null
> > > +++ b/fs/fat/fat_test.c
> > > @@ -0,0 +1,197 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * KUnit tests for FAT filesystems.
> > > + *
> > > + * Copyright (C) 2020 Google LLC.
> >
> > Nit: I know you wrote this last year, but I have had other maintainers
> > tell me the Copyright date should be set to when the final version of
> > the patch is sent out.
> >
> > I personally don't care, and I don't think you should resend this patch
> > just for that, but figured I would mention.
> >
>
> Hmm... I've definitely heard this both ways, but I can easily update
> the year if I need to send a new version out.
>
> > > + * Author: David Gow <davidgow@google.com>
> > > + */
> > > +
> > > +#include <kunit/test.h>
> > > +
> > > +#include "fat.h"
> > > +
> > > +static void fat_checksum_test(struct kunit *test)
> > > +{
> > > +     /* With no extension. */
> > > +     KUNIT_EXPECT_EQ(test, fat_checksum("VMLINUX    "), (u8)44);
> > > +     /* With 3-letter extension. */
> > > +     KUNIT_EXPECT_EQ(test, fat_checksum("README  TXT"), (u8)115);
> > > +     /* With short (1-letter) extension. */
> > > +     KUNIT_EXPECT_EQ(test, fat_checksum("ABCDEFGHA  "), (u8)98);
> >
> > How do you get the magic values? Or is this just supposed to be a
> > regression test?
>
> This is mainly meant to be a regression test, and the values did
> originally come from just running fat_checksum. I have, however,
> checked that Windows 98 produces the same values (on a FAT12
> filesystem).

All the above sounds good to me. Like I said before, all my comments
are pretty minor, I don't think you need to send a new revision for
those.

Re: [PATCH v8] fat: Add KUnit tests for checksums and timestamps

[Daniel Latypov]

On Thu, Apr 15, 2021 at 11:56 PM 'David Gow' via KUnit Development
<kunit-dev@googlegroups.com> wrote:
>
> Add some basic sanity-check tests for the fat_checksum() function and
> the fat_time_unix2fat() and fat_time_fat2unix() functions. These unit
> tests verify these functions return correct output for a number of test
> inputs.
>
> These tests were inspored by -- and serve a similar purpose to -- the
> timestamp parsing KUnit tests in ext4[1].
>
> Note that, unlike fat_time_unix2fat, fat_time_fat2unix wasn't previously
> exported, so this patch exports it as well. This is required for the
> case where we're building the fat and fat_test as modules.
>
> [1]:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/ext4/inode-test.c
>
> Signed-off-by: David Gow <davidgow@google.com>
> Acked-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Tested-by: Daniel Latypov <dlatypov@google.com>

The KUnit side of things looks good to me, added some minor nits below.
I had been hoping to run this using coverage, but something about that
is newly broken for my machine, so I haven't been able to :|

Because I felt committed to checking the coverage somehow, I added
some pr_info()s to see which branches are being taken.
We're hitting all of them except for the two in fat_time_unix2fat:
* the one for dates before 1980
* the one for dates after 2107

We have the earliest and latest possible dates as test cases already,
so it's probably not that important to hit those.

> ---
>
> It's been a while, but this hopefully is a final version of the FAT KUnit
> patchset. It has a number of changes to keep it up-to-date with current
> KUnit standards, notably the use of parameterised tests and the addition
> of a '.kunitconfig' file to allow for easy testing. It also fixes an
> endianness tagging issue picked up by the kernel test robot under sparse
> on pa-risc.
>
> Cheers,
> -- David
>
> Changes since v7:
> https://lore.kernel.org/linux-kselftest/20201028064631.3774908-1-davidgow@google.com/
> - Make the two timestamp tests parameterised: this means that the KUnit
>   runtime and tooling are aware of the different testcases (and print a
>   nice list of them to the TAP log when the test is run).
> - Fix some issues sparse picked up with __le32 tagged integers.
> - Add an fs/fat/.kunitconfig file which contains all the Kconfig entries
>   needed to run the test. The test can now be run with:
>   ./tools/testing/kunit/kunit.py run --kunitconfig fs/fat/.kunitconfig

FYI, if you do rebase send out a new revision, the test can be run via
$ ./tools/testing/kunit/kunit.py run --kunitconfig fs/fat

>
> Changes since v6:
> https://lore.kernel.org/linux-kselftest/20201024060558.2556249-1-davidgow@google.com/
> - Make CONFIG_FAT_DEFAULT_CODEPAGE depend on FAT_FS, rather than either
>   VFAT_FS or MSDOS_FS.
>   - This means that FAT_KUNIT_TEST can now also just depend of FAT_FS
> - Fix a few warnings that KUnit tool was eating:
>   - KUnit's type checking needs a specific cast for the fat_checksum()
>     expected results.
>   - The time test cases shouldn't be 'const'
>   - The fake superblock is now static, as otherwise it increased the
>     stack size too much.
>
> Changes since v4/5:
> https://lore.kernel.org/linux-kselftest/20201024052047.2526780-1-davidgow@google.com/
> - Fix a typo introduced in the Kconfig. It builds now.
>
> Changes since v3:
> https://lore.kernel.org/linux-kselftest/20201021061713.1545931-1-davidgow@google.com/
> - Update the Kconfig entry to use "depends on" rather than "select", as
>   discussed in [2].
> - Depend on "MSDOS_FS || VFAT_FS", rather than "FAT_FS", as we need the
>   CONFIG_FAT_DEFAULT_CODEPAGE symbol to be defined.
>
> Changes since v2:
> https://lore.kernel.org/linux-kselftest/20201020055856.1270482-1-davidgow@google.com/
> - Comment that the export for fat_time_fat2unix() function is for KUnit
>   tests.
>
> Changes since v1:
> https://lore.kernel.org/linux-kselftest/20201017064107.375174-1-davidgow@google.com/
> - Now export fat_time_fat2unix() so that the test can access it when
>   built as a module.
>
>
> [2]:
> https://lore.kernel.org/linux-ext4/52959e99-4105-3de9-730c-c46894b82bdd@infradead.org/T/#t
>
>
>
>  fs/fat/.kunitconfig |   5 ++
>  fs/fat/Kconfig      |  14 +++-
>  fs/fat/Makefile     |   2 +
>  fs/fat/fat_test.c   | 197 ++++++++++++++++++++++++++++++++++++++++++++
>  fs/fat/misc.c       |   2 +
>  5 files changed, 219 insertions(+), 1 deletion(-)
>  create mode 100644 fs/fat/.kunitconfig
>  create mode 100644 fs/fat/fat_test.c
>
> diff --git a/fs/fat/.kunitconfig b/fs/fat/.kunitconfig
> new file mode 100644
> index 000000000000..0a6971dbeccb
> --- /dev/null
> +++ b/fs/fat/.kunitconfig
> @@ -0,0 +1,5 @@
> +CONFIG_KUNIT=y
> +CONFIG_FAT_FS=y
> +CONFIG_MSDOS_FS=y
> +CONFIG_VFAT_FS=y
> +CONFIG_FAT_KUNIT_TEST=y
> diff --git a/fs/fat/Kconfig b/fs/fat/Kconfig
> index 66532a71e8fd..238cc55f84c4 100644
> --- a/fs/fat/Kconfig
> +++ b/fs/fat/Kconfig
> @@ -77,7 +77,7 @@ config VFAT_FS
>
>  config FAT_DEFAULT_CODEPAGE
>         int "Default codepage for FAT"
> -       depends on MSDOS_FS || VFAT_FS
> +       depends on FAT_FS
>         default 437
>         help
>           This option should be set to the codepage of your FAT filesystems.
> @@ -115,3 +115,15 @@ config FAT_DEFAULT_UTF8
>           Say Y if you use UTF-8 encoding for file names, N otherwise.
>
>           See <file:Documentation/filesystems/vfat.rst> for more information.
> +
> +config FAT_KUNIT_TEST
> +       tristate "Unit Tests for FAT filesystems" if !KUNIT_ALL_TESTS
> +       depends on KUNIT && FAT_FS
> +       default KUNIT_ALL_TESTS
> +       help
> +         This builds the FAT KUnit tests
> +
> +         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/fat/Makefile b/fs/fat/Makefile
> index 70645ce2f7fc..2b034112690d 100644
> --- a/fs/fat/Makefile
> +++ b/fs/fat/Makefile
> @@ -10,3 +10,5 @@ obj-$(CONFIG_MSDOS_FS) += msdos.o
>  fat-y := cache.o dir.o fatent.o file.o inode.o misc.o nfs.o
>  vfat-y := namei_vfat.o
>  msdos-y := namei_msdos.o
> +
> +obj-$(CONFIG_FAT_KUNIT_TEST) += fat_test.o
> diff --git a/fs/fat/fat_test.c b/fs/fat/fat_test.c
> new file mode 100644
> index 000000000000..febd25f57d4b
> --- /dev/null
> +++ b/fs/fat/fat_test.c
> @@ -0,0 +1,197 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * KUnit tests for FAT filesystems.
> + *
> + * Copyright (C) 2020 Google LLC.
> + * Author: David Gow <davidgow@google.com>
> + */
> +
> +#include <kunit/test.h>
> +
> +#include "fat.h"
> +
> +static void fat_checksum_test(struct kunit *test)
> +{
> +       /* With no extension. */
> +       KUNIT_EXPECT_EQ(test, fat_checksum("VMLINUX    "), (u8)44);
> +       /* With 3-letter extension. */
> +       KUNIT_EXPECT_EQ(test, fat_checksum("README  TXT"), (u8)115);
> +       /* With short (1-letter) extension. */
> +       KUNIT_EXPECT_EQ(test, fat_checksum("ABCDEFGHA  "), (u8)98);
> +}
> +
> +
> +struct fat_timestamp_testcase {
> +       const char *name;
> +       struct timespec64 ts;
> +       __le16 time;
> +       __le16 date;
> +       u8 cs;
> +       int time_offset;

Optional: it could be easier to read if we grouped the fields together, e.g.

struc timespec64 ts;
/* fields used by FAT */
time, date, cs;
int time_offset;

Or we could add a test-only struct

struct fat_timestamp {
  time, date, cs;
};

to keep things more readable, e.g. in unix2fat_test

struct fat_timestamp got;
...
KUNIT_EXPECT_EQ_MSG(test, test_case->cs, got.cs, "...")


> +};
> +
> +static struct fat_timestamp_testcase time_test_cases[] = {
> +       {
> +               .name = "Earliest possible UTC (1980-01-01 00:00:00)",
> +               .ts = {.tv_sec = 315532800LL, .tv_nsec = 0L},
> +               .time = cpu_to_le16(0),
> +               .date = cpu_to_le16(33),
> +               .cs = 0,
> +               .time_offset = 0,
> +       },
> +       {
> +               .name = "Latest possible UTC (2107-12-31 23:59:58)",
> +               .ts = {.tv_sec = 4354819198LL, .tv_nsec = 0L},
> +               .time = cpu_to_le16(49021),
> +               .date = cpu_to_le16(65439),
> +               .cs = 0,
> +               .time_offset = 0,
> +       },
> +       {
> +               .name = "Earliest possible (UTC-11) (== 1979-12-31 13:00:00 UTC)",
> +               .ts = {.tv_sec = 315493200LL, .tv_nsec = 0L},
> +               .time = cpu_to_le16(0),
> +               .date = cpu_to_le16(33),
> +               .cs = 0,
> +               .time_offset = 11 * 60,
> +       },
> +       {
> +               .name = "Latest possible (UTC+11) (== 2108-01-01 10:59:58 UTC)",
> +               .ts = {.tv_sec = 4354858798LL, .tv_nsec = 0L},
> +               .time = cpu_to_le16(49021),
> +               .date = cpu_to_le16(65439),
> +               .cs = 0,
> +               .time_offset = -11 * 60,
> +       },
> +       {
> +               .name = "Leap Day / Year (1996-02-29 00:00:00)",
> +               .ts = {.tv_sec = 825552000LL, .tv_nsec = 0L},
> +               .time = cpu_to_le16(0),
> +               .date = cpu_to_le16(8285),
> +               .cs = 0,
> +               .time_offset = 0,
> +       },
> +       {
> +               .name = "Year 2000 is leap year (2000-02-29 00:00:00)",
> +               .ts = {.tv_sec = 951782400LL, .tv_nsec = 0L},
> +               .time = cpu_to_le16(0),
> +               .date = cpu_to_le16(10333),
> +               .cs = 0,
> +               .time_offset = 0,
> +       },
> +       {
> +               .name = "Year 2100 not leap year (2100-03-01 00:00:00)",
> +               .ts = {.tv_sec = 4107542400LL, .tv_nsec = 0L},
> +               .time = cpu_to_le16(0),
> +               .date = cpu_to_le16(61537),
> +               .cs = 0,
> +               .time_offset = 0,
> +       },
> +       {
> +               .name = "Leap year + timezone UTC+1 (== 2004-02-29 00:30:00 UTC)",
> +               .ts = {.tv_sec = 1078014600LL, .tv_nsec = 0L},
> +               .time = cpu_to_le16(48064),
> +               .date = cpu_to_le16(12380),
> +               .cs = 0,
> +               .time_offset = -60,
> +       },
> +       {
> +               .name = "Leap year + timezone UTC-1 (== 2004-02-29 23:30:00 UTC)",
> +               .ts = {.tv_sec = 1078097400LL, .tv_nsec = 0L},
> +               .time = cpu_to_le16(960),
> +               .date = cpu_to_le16(12385),
> +               .cs = 0,
> +               .time_offset = 60,
> +       },
> +       {
> +               .name = "VFAT odd-second resolution (1999-12-31 23:59:59)",
> +               .ts = {.tv_sec = 946684799LL, .tv_nsec = 0L},
> +               .time = cpu_to_le16(49021),
> +               .date = cpu_to_le16(10143),
> +               .cs = 100,
> +               .time_offset = 0,
> +       },
> +       {
> +               .name = "VFAT 10ms resolution (1980-01-01 00:00:00:0010)",
> +               .ts = {.tv_sec = 315532800LL, .tv_nsec = 10000000L},
> +               .time = cpu_to_le16(0),
> +               .date = cpu_to_le16(33),
> +               .cs = 1,
> +               .time_offset = 0,
> +       },
> +};
> +
> +static void time_testcase_desc(struct fat_timestamp_testcase *t,
> +                              char *desc)
> +{
> +       strscpy(desc, t->name, KUNIT_PARAM_DESC_SIZE);
> +}
> +
> +KUNIT_ARRAY_PARAM(fat_time, time_test_cases, time_testcase_desc);
> +
> +static void fat_time_fat2unix_test(struct kunit *test)
> +{
> +       static struct msdos_sb_info fake_sb;
> +       struct timespec64 ts;
> +       struct fat_timestamp_testcase *testcase =
> +               (struct fat_timestamp_testcase *)test->param_value;
> +
> +       fake_sb.options.tz_set = 1;
> +       fake_sb.options.time_offset = testcase->time_offset;
> +
> +       fat_time_fat2unix(&fake_sb, &ts,
> +                         testcase->time,
> +                         testcase->date,
> +                         testcase->cs);
> +       KUNIT_EXPECT_EQ_MSG(test,
> +                           testcase->ts.tv_sec,
> +                           ts.tv_sec,
> +                           "Timestamp mismatch (seconds)\n");

Nit: if I delete these \n's, I don't see a difference in kunit.py output.
I also don't think I see a difference in the raw output either.

If I put \n\n, then I'll see a difference.

So should we drop these single trailing \n?

> +       KUNIT_EXPECT_EQ_MSG(test,
> +                           testcase->ts.tv_nsec,
> +                           ts.tv_nsec,
> +                           "Timestamp mismatch (nanoseconds)\n");
> +}
> +
> +static void fat_time_unix2fat_test(struct kunit *test)
> +{
> +       static struct msdos_sb_info fake_sb;
> +       __le16 date, time;
> +       u8 cs;
> +       struct fat_timestamp_testcase *testcase =
> +               (struct fat_timestamp_testcase *)test->param_value;
> +
> +       fake_sb.options.tz_set = 1;
> +       fake_sb.options.time_offset = testcase->time_offset;
> +
> +       fat_time_unix2fat(&fake_sb, &(testcase->ts),
> +                         &time, &date, &cs);
> +       KUNIT_EXPECT_EQ_MSG(test,
> +                           le16_to_cpu(testcase->time),
> +                           le16_to_cpu(time),
> +                           "Time mismatch\n");
> +       KUNIT_EXPECT_EQ_MSG(test,
> +                           le16_to_cpu(testcase->date),
> +                           le16_to_cpu(date),
> +                           "Date mismatch\n");
> +       KUNIT_EXPECT_EQ_MSG(test,
> +                           testcase->cs,
> +                           cs,
> +                           "Centisecond mismatch\n");
> +}
> +
> +static struct kunit_case fat_test_cases[] = {
> +       KUNIT_CASE(fat_checksum_test),
> +       KUNIT_CASE_PARAM(fat_time_fat2unix_test, fat_time_gen_params),
> +       KUNIT_CASE_PARAM(fat_time_unix2fat_test, fat_time_gen_params),
> +       {},
> +};
> +
> +static struct kunit_suite fat_test_suite = {
> +       .name = "fat_test",
> +       .test_cases = fat_test_cases,
> +};
> +
> +kunit_test_suites(&fat_test_suite);
> +
> +MODULE_LICENSE("GPL v2");
> diff --git a/fs/fat/misc.c b/fs/fat/misc.c
> index 18a50a46b57f..9073fa927be3 100644
> --- a/fs/fat/misc.c
> +++ b/fs/fat/misc.c
> @@ -229,6 +229,8 @@ void fat_time_fat2unix(struct msdos_sb_info *sbi, struct timespec64 *ts,
>                 ts->tv_nsec = 0;
>         }
>  }
> +/* Export fat_time_fat2unix() for the fat_test KUnit tests. */
> +EXPORT_SYMBOL_GPL(fat_time_fat2unix);
>
>  /* Convert linear UNIX date to a FAT time/date pair. */
>  void fat_time_unix2fat(struct msdos_sb_info *sbi, struct timespec64 *ts,
> --
> 2.31.1.368.gbe11c130af-goog
>
> --
> You received this message because you are subscribed to the Google Groups "KUnit Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20210416065623.882364-1-davidgow%40google.com.