lkml.org 
[lkml]   [2023]   [Feb]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v1 1/3] kunit: fix bug in debugfs logs of parameterized tests
    On Wed, 1 Feb 2023 at 06:04, Rae Moar <rmoar@google.com> wrote:
    >
    > Fix bug in debugfs logs that causes parameterized results to not appear
    > in the log because the log is reintialized (cleared) when each parameter is

    Nit: s/reintialized/reinitialized

    > run.
    >
    > Ensure these results appear in the debugfs logs and increase log size to
    > allow for the size of parameterized results.
    >
    > Signed-off-by: Rae Moar <rmoar@google.com>
    > ---

    This looks pretty good to me, but we may need to restrict the size of
    a single log line separately from that of the whole log.

    (It'd also be nice to include a before/after in the commit description.)

    With the stack size issue fixed, though, this looks good to me:
    Reviewed-by: David Gow <davidgow@google.com>

    Cheers,
    -- David

    > include/kunit/test.h | 2 +-
    > lib/kunit/test.c | 3 ++-
    > 2 files changed, 3 insertions(+), 2 deletions(-)
    >
    > diff --git a/include/kunit/test.h b/include/kunit/test.h
    > index 87ea90576b50..0a077a4c067c 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

    This is used both as the overall log size, and the size of a single
    line in kunit_log_append.

    As the latter involves a local 'line' array, it can bloat out stack usage.

    Could we either restrict the maximum line length separately, or rework
    kunit_log_append() to not use a local variable?
    (I imagine we could just vsnprintf() directly into the log buffer. We
    could make two sprintf calls to calculate the length required to
    maintain some atomicity as well (this could open us up to
    time-of-check/time-of-use vulnerabilities, but I think the
    vulnerability ship has sailed if you're passing untrusted pointers
    into the kunit_log macro anyway))

    >
    > /* 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 51cae59d8aae..66ba93b8222c 100644
    > --- a/lib/kunit/test.c
    > +++ b/lib/kunit/test.c
    > @@ -437,7 +437,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 +532,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);
    > --
    > 2.39.1.456.gfc5497dd1b-goog
    >
    [unhandled content-type:application/pkcs7-signature]
    \
     
     \ /
      Last update: 2023-03-27 00:15    [W:2.866 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site