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.

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.

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.

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@go...

• 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@go...

• 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@go...

• Fix a typo introduced in the Kconfig. It builds now.

Changes since v3: https://lore.kernel.org/linux-kselftest/20201021061713.1545931-1-davidgow@go...

• 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@go...

• 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@goo...

• Now export fat_time_fat2unix() so that the test can access it when built as a module.

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....