linux-kernel - Re: [PATCH v4 07/10] kunit: string-stream: Decouple string

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: <CABVgOS=1Vp+bLidUsK7Dv+3=0x0FE3xJzaQH53nzrdBknMLu-A@mail.gmail.com>
Date:   Thu, 17 Aug 2023 14:24:28 +0800
From:   David Gow <davidgow@...gle.com>
To:     Richard Fitzgerald <rf@...nsource.cirrus.com>
Cc:     brendan.higgins@...ux.dev, rmoar@...gle.com,
        linux-kselftest@...r.kernel.org, kunit-dev@...glegroups.com,
        linux-kernel@...r.kernel.org, patches@...nsource.cirrus.com
Subject: Re: [PATCH v4 07/10] kunit: string-stream: Decouple string_stream
 from kunit

On Tue, 15 Aug 2023 at 18:51, Richard Fitzgerald
<rf@...nsource.cirrus.com> wrote:
>
> On 15/8/23 10:16, David Gow wrote:
> > On Mon, 14 Aug 2023 at 21:23, Richard Fitzgerald
> > <rf@...nsource.cirrus.com> wrote:
> >>
> >> Re-work string_stream so that it is not tied to a struct kunit. This is
> >> to allow using it for the log of struct kunit_suite.
> >>
> >> Instead of resource-managing individual allocations the whole string_stream
> >> object can be resource-managed as a single object:
> >>
> >>      alloc_string_stream() API is unchanged and takes a pointer to a
> >>      struct kunit but it now registers the returned string_stream object to
> >>      be resource-managed.
> >>
> >>      raw_alloc_string_stream() is a new function that allocates a
> >>      bare string_stream without any association to a struct kunit.
> >>
> >>      free_string_stream() is a new function that frees a resource-managed
> >>      string_stream allocated by alloc_string_stream().
> >>
> >>      raw_free_string_stream() is a new function that frees a non-managed
> >>      string_stream allocated by raw_alloc_string_stream().
> >>
> >> The confusing function string_stream_destroy() has been removed. This only
> >> called string_stream_clear() but didn't free the struct string_stream.
> >> Instead string_stream_clear() has been exported, and the new functions use
> >> the more conventional naming of "free" as the opposite of "alloc".
> >>
> >> Signed-off-by: Richard Fitzgerald <rf@...nsource.cirrus.com>
> >> ---
> >
> > I'm in favour of this. Should we go further and get rid of the struct
> > kunit member from string_stream totally?
> >
>
> I can do that. I was worried about some hairy-looking code in assert.c
> that used stream->test. But I've just looked at it again and it's
> really quite simple, and doesn't even need ->test. is_literal()
> allocates a temporary managed buffer, but it frees it before returning
> so it doesn't need to be managed.
>

Yeah, let's get rid of that. Having a stream->kunit exist but be NULL
half the time is asking for issues down the line.

> > Also, note that the kunit_action_t casting is causing warnings on some
> > clang configs (and technically isn't valid C). Personally, I still
> > like it, but expect more emails from the kernel test robot and others.
> >
>
> I can send a new version to fix that.
>

That's probably best. If you want to keep it as-is, I'll fight for it,
but it's probably better to err on the side of not introducing the
warnings.

Thanks,
-- David

Download attachment "smime.p7s" of type "application/pkcs7-signature" (4003 bytes)