Re: [PATCH v3 1/3] kunit: fix bug in debugfs logs of parameterized tests

[David Gow]

On Wed, 8 Mar 2023 at 06:39, Rae Moar <rmoar@xxxxxxxxxx> wrote:
>
> Fix bug in debugfs logs that causes individual parameterized results to not
> appear because the log is reinitialized (cleared) when each parameter is
> run.
>
> Ensure these results appear in the debugfs logs, increase log size to
> allow for the size of parameterized results. As a result, append lines to
> the log directly rather than using an intermediate variable that can cause
> stack size warnings due to the increased log size.
>
> Here is the debugfs log of ext4_inode_test which uses parameterized tests
> before the fix:
>
>      KTAP version 1
>
>      # Subtest: ext4_inode_test
>      1..1
>  # Totals: pass:16 fail:0 skip:0 total:16
>  ok 1 ext4_inode_test
>
> As you can see, this log does not include any of the individual
> parametrized results.
>
> After (in combination with the next two fixes to remove extra empty line
> and ensure KTAP valid format):
>
>  KTAP version 1
>  1..1
>      KTAP version 1
>      # Subtest: ext4_inode_test
>      1..1
>         KTAP version 1
>          # Subtest: inode_test_xtimestamp_decoding
>          ok 1 1901-12-13 Lower bound of 32bit < 0 timestamp, no extra bits
>          ... (the rest of the individual parameterized tests)
>          ok 16 2446-05-10 Upper bound of 32bit >=0 timestamp. All extra
>      # inode_test_xtimestamp_decoding: pass:16 fail:0 skip:0 total:16
>      ok 1 inode_test_xtimestamp_decoding
>  # Totals: pass:16 fail:0 skip:0 total:16
>  ok 1 ext4_inode_test
>
> Signed-off-by: Rae Moar <rmoar@xxxxxxxxxx>
> Reviewed-by: David Gow <davidgow@xxxxxxxxxx>
> ---

Thanks, this is working fine here!

Reviewed-by: David Gow <davidgow@xxxxxxxxxx>

Cheers,
-- David





>
> Changes from v2 -> v3:
> - Fix a off-by-one bug in the kunit_log_append method.
>
> Changes from v1 -> v2:
> - Remove the use of the line variable in kunit_log_append that was causing
>   stack size warnings.
> - Add before and after to the commit message.
>
>  include/kunit/test.h |  2 +-
>  lib/kunit/test.c     | 18 ++++++++++++------
>  2 files changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index 08d3559dd703..0668d29f3453 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -34,7 +34,7 @@ DECLARE_STATIC_KEY_FALSE(kunit_running);
>  struct kunit;
>
>  /* Size of log associated with test. */
> -#define KUNIT_LOG_SIZE 512
> +#define KUNIT_LOG_SIZE 1500
>
>  /* Maximum size of parameter description string. */
>  #define KUNIT_PARAM_DESC_SIZE 128
> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index c9e15bb60058..c4d6304edd61 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -114,22 +114,27 @@ static void kunit_print_test_stats(struct kunit *test,
>   */
>  void kunit_log_append(char *log, const char *fmt, ...)
>  {
> -       char line[KUNIT_LOG_SIZE];
>         va_list args;
> -       int len_left;
> +       int len, log_len, len_left;
>
>         if (!log)
>                 return;
>
> -       len_left = KUNIT_LOG_SIZE - strlen(log) - 1;
> +       log_len = strlen(log);
> +       len_left = KUNIT_LOG_SIZE - log_len - 1;
>         if (len_left <= 0)
>                 return;
>
> +       /* Evaluate length of line to add to log */
>         va_start(args, fmt);
> -       vsnprintf(line, sizeof(line), fmt, args);
> +       len = vsnprintf(NULL, 0, fmt, args) + 1;
> +       va_end(args);
> +
> +       /* Print formatted line to the log */
> +       va_start(args, fmt);
> +       vsnprintf(log + log_len, min(len, len_left), fmt, args);
>         va_end(args);
>
> -       strncat(log, line, len_left);
>  }
>  EXPORT_SYMBOL_GPL(kunit_log_append);
>
> @@ -437,7 +442,6 @@ static void kunit_run_case_catch_errors(struct kunit_suite *suite,
>         struct kunit_try_catch_context context;
>         struct kunit_try_catch *try_catch;
>
> -       kunit_init_test(test, test_case->name, test_case->log);
>         try_catch = &test->try_catch;
>
>         kunit_try_catch_init(try_catch,
> @@ -533,6 +537,8 @@ int kunit_run_tests(struct kunit_suite *suite)
>                 struct kunit_result_stats param_stats = { 0 };
>                 test_case->status = KUNIT_SKIPPED;
>
> +               kunit_init_test(&test, test_case->name, test_case->log);
> +
>                 if (!test_case->generate_params) {
>                         /* Non-parameterised test. */
>                         kunit_run_case_catch_errors(suite, test_case, &test);
>
> base-commit: 60684c2bd35064043360e6f716d1b7c20e967b7d
> --
> 2.40.0.rc0.216.gc4246ad0f0-goog
>

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature