lkml.org 
[lkml]   [2020]   [Dec]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2 2/4] lib: test_scanf: Add tests for sscanf number conversion
From
Date


On 09/12/2020 14:15, Petr Mladek wrote:
> On Mon 2020-11-30 14:57:58, Richard Fitzgerald wrote:
>> Adds test_sscanf to test various number conversion cases, as
>> number conversion was previously broken.
>>
>> This also tests the simple_strtoxxx() functions exported from
>> vsprintf.c.
>
> It is impressive.
>
> Honestly, I do not feel to be expert on testing and mathematics.
> I am not sure how comprehensive the test is. Also I am not
> sure what experts would say about the tricks with random
> numbers.
>
> Anyway, this is much more than what I have expected. And it checks
> great number of variants and corner cases.
>
> I suggest only one small change, see below.
>
>> --- /dev/null
>> +++ b/lib/test_scanf.c
>> @@ -0,0 +1,747 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Test cases for sscanf facility.
>> + */
>> +
>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>> +
>> +#include <linux/bitops.h>
>> +#include <linux/init.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/overflow.h>
>> +#include <linux/printk.h>
>> +#include <linux/random.h>
>> +#include <linux/slab.h>
>> +#include <linux/string.h>
>> +
>> +#include "../tools/testing/selftests/kselftest_module.h"
>> +
>> +#define BUF_SIZE 1024
>> +
>> +static unsigned total_tests __initdata;
>> +static unsigned failed_tests __initdata;
>> +static char *test_buffer __initdata;
>> +static char *fmt_buffer __initdata;
>> +static struct rnd_state rnd_state __initdata;
>> +
>> +typedef int (*check_fn)(const void *check_data, const char *string,
>> + const char *fmt, int n_args, va_list ap);
>> +
>> +static void __scanf(4, 6) __init
>> +_test(check_fn fn, const void *check_data, const char *string, const char *fmt,
>> + int n_args, ...)
>> +{
>> + va_list ap, ap_copy;
>> + int ret;
>> +
>> + total_tests++;
>> +
>> + va_start(ap, n_args);
>> + va_copy(ap_copy, ap);
>> + ret = vsscanf(string, fmt, ap_copy);
>> + va_end(ap_copy);
>> +
>> + if (ret != n_args) {
>> + pr_warn("vsscanf(\"%s\", \"%s\", ...) returned %d expected %d\n",
>> + string, fmt, ret, n_args);
>> + goto fail;
>> + }
>> +
>> + ret = (*fn)(check_data, string, fmt, n_args, ap);
>> + if (ret)
>> + goto fail;
>> +
>> + va_end(ap);
>> +
>> + return;
>> +
>> +fail:
>> + failed_tests++;
>> + va_end(ap);
>> +}
>> +
>> +#define test_one_number(T, gen_fmt, scan_fmt, val, fn) \
>> +do { \
>> + const T expect_val = (T)(val); \
>> + T result = ~expect_val; /* should be overwritten */ \
>
> If I get it correctly, this is supposed to initialize the temporary
> variable with a value that is different from the expected value.
> It will cause test failure when it is not updated by vsscanf().
>
> It does not work for zero value. A better solution might be to add

That's a ~, not a -
~0 = 0xFFFFFFFF
~-1 = 0

> a constant, for example:
>
> T result = expect_val + 3; /* do not match when not overwritten */ \
>
> I did not use "+ 1" intentionally because it might hide some overflow
> issues.
>
>> + \
>> + snprintf(test_buffer, BUF_SIZE, gen_fmt, expect_val); \
>> + _test(fn, &expect_val, test_buffer, "%" scan_fmt, 1, &result); \
>> +} while (0)
>
> Otherwise, it looks good to me.
>
> Best Regards,
> Petr
>

\
 
 \ /
  Last update: 2020-12-15 15:32    [W:0.047 / U:0.444 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site