| 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
| ||
|
Date: Thu, 10 Aug 2023 15:00:39 +0100
From: Richard Fitzgerald <rf@...nsource.cirrus.com>
To: Rae Moar <rmoar@...gle.com>
CC: <brendan.higgins@...ux.dev>, <davidgow@...gle.com>,
<linux-kselftest@...r.kernel.org>, <kunit-dev@...glegroups.com>,
<linux-kernel@...r.kernel.org>, <patches@...nsource.cirrus.com>
Subject: Re: [PATCH v3 1/7] kunit: Replace fixed-size log with
dynamically-extending buffer
On 9/8/23 22:10, Rae Moar wrote:
> On Wed, Aug 9, 2023 at 11:54 AM Richard Fitzgerald
> <rf@...nsource.cirrus.com> wrote:
>>
>> Re-implement the log buffer as a list of buffer fragments that can
>> be extended as the size of the log info grows.
>>
>> When using parameterization the test case can run many times and create
>> a large amount of log. It's not really practical to keep increasing the
>> size of the fixed buffer every time a test needs more space. And a big
>> fixed buffer wastes memory.
>>
>> The original char *log pointer is replaced by a pointer to a list of
>> struct kunit_log_frag, each containing a fixed-size buffer.
>>
>> kunit_log_append() now attempts to append to the last kunit_log_frag in
>> the list. If there isn't enough space it will append a new kunit_log_frag
>> to the list. This simple implementation does not attempt to completely
>> fill the buffer in every kunit_log_frag.
>>
>> The 'log' member of kunit_suite, kunit_test_case and kunit_suite must be a
>> pointer because the API of kunit_log() requires that is the same type in
>> all three structs. As kunit.log is a pointer to the 'log' of the current
>> kunit_case, it must be a pointer in the other two structs.
>>
>> The existing kunit-test.c log tests have been updated to build against the
>> new fragmented log implementation. If the test fails the new function
>> get_concatenated_log() constructs a single contiguous string from the
>> log fragments so that the whole log can be emitted in the failure
>> message.
>
> Hello!
>
> All the tests now pass for me and this patch now looks good to me. I
> have tested it and it seems to be working well.
>
> I just have a few nits below. But I am overall happy with this patch.
>
> Reviewed-by: Rae Moar <rmoar@...gle.com>
>
> -Rae
>
...
>> +static char *get_concatenated_log(struct kunit *test, const struct list_head *log)
>> +{
>> + struct kunit_log_frag *frag;
>> + size_t len = 0;
>> + char *p;
>
> I wonder if we could change the name of p to be a bit more
> descriptive. Maybe concat_log?
I'll do that.
>> +
>> + list_for_each_entry(frag, log, list)
>> + len += strlen(frag->buf);
>> +
>> + len++; /* for terminating '\0' */
>> + p = kunit_kzalloc(test, len, GFP_KERNEL);
>> +
>> + list_for_each_entry(frag, log, list)
>> + strlcat(p, frag->buf, len);
>> +
>> + return p;
>> +}
>> +
>> static void kunit_log_test(struct kunit *test)
>> {
>> struct kunit_suite suite;
>> + struct kunit_log_frag *frag;
>>
>> - suite.log = kunit_kzalloc(test, KUNIT_LOG_SIZE, GFP_KERNEL);
>> + suite.log = kunit_kzalloc(test, sizeof(*suite.log), GFP_KERNEL);
>> KUNIT_ASSERT_NOT_ERR_OR_NULL(test, suite.log);
>> + INIT_LIST_HEAD(suite.log);
>
> This section of the test is pretty dense. I would love to see at least
> one comment in this section. Maybe it could be here and say something
> like:
> "/* Allocate, initialize, and then add the first fragment of log */"
Good. You like comments. So do I.
But many people hate them so when I'm starting on a new subsystem I go
with however the existing code looks. And the bits of kunit I've looked
at are light on comments.
...
>> @@ -543,14 +568,17 @@ static void kunit_log_test(struct kunit *test)
>> kunit_log(KERN_INFO, &suite, "along with this.");
>>
>> #ifdef CONFIG_KUNIT_DEBUGFS
>> + frag = list_first_entry(test->log, struct kunit_log_frag, list);
>> KUNIT_EXPECT_NOT_ERR_OR_NULL(test,
>> - strstr(test->log, "put this in log."));
>> + strstr(frag->buf, "put this in log."));
>> KUNIT_EXPECT_NOT_ERR_OR_NULL(test,
>> - strstr(test->log, "this too."));
>> + strstr(frag->buf, "this too."));
>> +
>> + frag = list_first_entry(suite.log, struct kunit_log_frag, list);
>> KUNIT_EXPECT_NOT_ERR_OR_NULL(test,
>> - strstr(suite.log, "add to suite log."));
>> + strstr(frag->buf, "add to suite log."));
>> KUNIT_EXPECT_NOT_ERR_OR_NULL(test,
>> - strstr(suite.log, "along with this."));
>> + strstr(frag->buf, "along with this."));
>> #else
>> KUNIT_EXPECT_NULL(test, test->log);
>
> This test passes when CONFIG_KUNIT_DEBUGFS=n while most of the other
> tests are skipped. Should this test be skipped instead?
>
> I'm assuming since the assert/expect statements at the beginning are
> run even if CONFIG_KUNIT_DEBUGFS=n, this test should not be skipped
> but I just wanted to check.
That the existing code, which I didn't change.
I think the best thing here would be to move the fragment testing into a
new test case.
>> #endif
>> @@ -558,11 +586,15 @@ static void kunit_log_test(struct kunit *test)
>>
>> static void kunit_log_newline_test(struct kunit *test)
>> {
>> + struct kunit_log_frag *frag;
>> +
>> kunit_info(test, "Add newline\n");
>> if (test->log) {
>
> This is a small nit but I would prefer that the if statements to
> decide whether CONFIG_KUNIT_DEBUGFS is enabled were uniform. So I
> would prefer that we choose between if (test->log) and #ifdef
> CONFIG_KUNIT_DEBUGFS. I think we originally chose to use if
> (test->log) here to avoid the compile-time #ifdef.
Actually the existing code did it both ways. The newline test used
if (test->log) - but this makes sense because it actually does testing
on test->log.
The original kunit_log_test() used #ifdef CONFIG_KUNIT_DEBUGFS. I based
my new functions on kunit_log_test(). But I can change them. Would you
prefer
if (!test->log)
skip
or
if (!IS_ENABLED(CONFIG_KUNIT_DEBUGFS))
skip
Powered by blists - more mailing lists