linux-kernel - Re: [PATCH 2/3] kunit: add ability to specify suite-level init and exit functions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CABVgOSnb0CveWUqbB8aSYu53uRmi+H9Dim3mYbyXi+eQo=y4ww@mail.gmail.com>
Date:   Fri, 29 Apr 2022 14:01:40 +0800
From:   David Gow <davidgow@...gle.com>
To:     Daniel Latypov <dlatypov@...gle.com>
Cc:     Brendan Higgins <brendanhiggins@...gle.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        KUnit Development <kunit-dev@...glegroups.com>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@...r.kernel.org>,
        Shuah Khan <skhan@...uxfoundation.org>
Subject: Re: [PATCH 2/3] kunit: add ability to specify suite-level init and
 exit functions

On Wed, Apr 27, 2022 at 11:06 AM Daniel Latypov <dlatypov@...gle.com> wrote:
>
> On Tue, Apr 26, 2022 at 8:56 PM David Gow <davidgow@...gle.com> wrote:
> > >
> > >  static size_t kunit_suite_counter = 1;
> > >
> > > -static void kunit_print_suite_end(struct kunit_suite *suite)
> > > +static void kunit_print_suite_end(struct kunit_suite *suite, int init_err)
> >
> > A part of me feels that it'd be nicer to have the init_err be part of
> > struct kunit_suite, and have kunit_suite_has_succeeded() take it into
> > account. It could go either way, though -- WDYT?
>
> Yeah, passing it around as a parameter felt a bit icky.
> But I think adding it in as a field feels worse.

Personally, I don't have a problem with having it as a field (other
than the memory usage, which shouldn't be so much as to cause
problems). It seems that the suite result is logically part of the
suite, and given that status_comment is in struct kunit_suite and
there's a kunit_status field in kunit_case, having it as a field in
the suite seems the most logically consistent thing to do.

>
> Another thought: perhaps have this function take a `kunit_status`
> parameter instead?
> Moving the ?: expression below out into the caller isn't that bad, imo.

It still doesn't solve the fact that kunit_suite_has_succeeded() no
longer tells you if a suite has succeeded or not. If we stick with
this (with the conditional either here or in the caller), I think we
should at least rename kunit_suite_has_succeded() to something like
kunit_suite_subtests_status().

That being said, I do prefer passing a 'kunit_status' around to an 'int'.

>
> >
> >
> > >  {
> > > +       enum kunit_status status =
> > > +               init_err ? KUNIT_FAILURE : kunit_suite_has_succeeded(suite);
> > > +