Messages in this thread | | | Subject | Re: [PATCH v2 2/4] lib: test_scanf: Add tests for sscanf number conversion | From | Richard Fitzgerald <> | Date | Tue, 15 Dec 2020 14:26:53 +0000 |
| |
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 >
| |