Re: [PATCH v5 bpf-next 4/4] selftest/bpf: Extend the bpf_snprintf() test for "%c".

From:   Andrii Nakryiko <redacted>
Date:   Fri, 13 Aug 2021 16:30:29 -0700

On Fri, Aug 13, 2021 at 4:28 PM Andrii Nakryiko
[off-list ref] wrote:

quoted

On Thu, Aug 12, 2021 at 9:47 AM Kuniyuki Iwashima [off-list ref] wrote:

quoted

This patch adds a "positive" pattern for "%c", which intentionally uses a
__u32 value (0x64636261, "dbca") to print a single character "a".  If the
implementation went wrong, other 3 bytes might show up as the part of the
latter "%+05s".

Also, this patch adds two "negative" patterns for wide character.

Signed-off-by: Kuniyuki Iwashima <redacted>
---
 tools/testing/selftests/bpf/prog_tests/snprintf.c | 4 +++-
 tools/testing/selftests/bpf/progs/test_snprintf.c | 7 ++++---
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/snprintf.c b/tools/testing/selftests/bpf/prog_tests/snprintf.c
index dffbcaa1ec98..f77d7def7fed 100644
--- a/tools/testing/selftests/bpf/prog_tests/snprintf.c
+++ b/tools/testing/selftests/bpf/prog_tests/snprintf.c

@@ -19,7 +19,7 @@
 #define EXP_ADDR_OUT "0000000000000000 ffff00000add4e55 "
 #define EXP_ADDR_RET sizeof(EXP_ADDR_OUT "unknownhashedptr")

-#define EXP_STR_OUT  "str1 longstr"
+#define EXP_STR_OUT  "str1         a longstr"
 #define EXP_STR_RET  sizeof(EXP_STR_OUT)

 #define EXP_OVER_OUT "%over"

@@ -114,6 +114,8 @@ void test_snprintf_negative(void)
        ASSERT_ERR(load_single_snprintf("%"), "invalid specifier 3");
        ASSERT_ERR(load_single_snprintf("%12345678"), "invalid specifier 4");
        ASSERT_ERR(load_single_snprintf("%--------"), "invalid specifier 5");
+       ASSERT_ERR(load_single_snprintf("%lc"), "invalid specifier 6");
+       ASSERT_ERR(load_single_snprintf("%llc"), "invalid specifier 7");
        ASSERT_ERR(load_single_snprintf("\x80"), "non ascii character");
        ASSERT_ERR(load_single_snprintf("\x1"), "non printable character");
 }

diff --git a/tools/testing/selftests/bpf/progs/test_snprintf.c b/tools/testing/selftests/bpf/progs/test_snprintf.c
index e2ad26150f9b..afc2c583125b 100644
--- a/tools/testing/selftests/bpf/progs/test_snprintf.c
+++ b/tools/testing/selftests/bpf/progs/test_snprintf.c

@@ -40,6 +40,7 @@ int handler(const void *ctx)
        /* Convenient values to pretty-print */
        const __u8 ex_ipv4[] = {127, 0, 0, 1};
        const __u8 ex_ipv6[] = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1};
+       const __u32 chr1 = 0x64636261; /* dcba */
        static const char str1[] = "str1";
        static const char longstr[] = "longstr";

@@ -59,9 +60,9 @@ int handler(const void *ctx)
        /* Kernel pointers */
        addr_ret = BPF_SNPRINTF(addr_out, sizeof(addr_out), "%pK %px %p",
                                0, 0xFFFF00000ADD4E55, 0xFFFF00000ADD4E55);
-       /* Strings embedding */
-       str_ret  = BPF_SNPRINTF(str_out, sizeof(str_out), "%s %+05s",
-                               str1, longstr);
+       /* Strings and single-byte character embedding */
+       str_ret  = BPF_SNPRINTF(str_out, sizeof(str_out), "%s % 9c %+05s",
+                               str1, chr1, longstr);

Can you also add tests for %+2c, %-3c, %04c, %0c? Think outside the box ;)

Sure.

quoted

Why this hackery with __u32? You are making an endianness assumption
(it will break on big-endian), and you'd never write real code like
that. Just pass 'a', what's wrong with that?

In my first implementation of "%c" support, I tried to support "%lc" and
"%llc" also and reused the later int code.  Then just testing 'a' was ok,
but it was wrong with the 0x64636261, three bytes of which showed up as
part of the next %s.  So, I thought it would be better to test with int.
But exactly it breaks the big-endian case, I'll just pass 'a'.

quoted

quoted

        /* Overflow */
        over_ret = BPF_SNPRINTF(over_out, sizeof(over_out), "%%overflow");
        /* Padding of fixed width numbers */
--
2.30.2