Netdev Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Kuniyuki Iwashima <kuniyu@amazon.co.jp>
To: <andrii.nakryiko@gmail.com>
Cc: <andrii@kernel.org>, <ast@kernel.org>, <benh@amazon.com>,
	<bpf@vger.kernel.org>, <daniel@iogearbox.net>,
	<davem@davemloft.net>, <john.fastabend@gmail.com>, <kafai@fb.com>,
	<kpsingh@kernel.org>, <kuba@kernel.org>, <kuni1840@gmail.com>,
	<kuniyu@amazon.co.jp>, <netdev@vger.kernel.org>,
	<songliubraving@fb.com>, <yhs@fb.com>
Subject: Re: [PATCH v5 bpf-next 4/4] selftest/bpf: Extend the bpf_snprintf() test for "%c".
Date: Sat, 14 Aug 2021 09:37:02 +0900	[thread overview]
Message-ID: <20210814003702.35395-1-kuniyu@amazon.co.jp> (raw)
In-Reply-To: <CAEf4Bzb-Y5SRrS6VHpBbosUj1QU+76zo29KOJF9-GBoJKaZhCQ@mail.gmail.com>

From:   Andrii Nakryiko <andrii.nakryiko@gmail.com>
Date:   Fri, 13 Aug 2021 16:30:29 -0700
> On Fri, Aug 13, 2021 at 4:28 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Thu, Aug 12, 2021 at 9:47 AM Kuniyuki Iwashima <kuniyu@amazon.co.jp> wrote:
> > >
> > > 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 <kuniyu@amazon.co.jp>
> > > ---
> > >  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.


> > 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'.


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

      reply	other threads:[~2021-08-14  0:37 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-12 16:45 [PATCH v5 bpf-next 0/4] BPF iterator for UNIX domain socket Kuniyuki Iwashima
2021-08-12 16:45 ` [PATCH v5 bpf-next 1/4] bpf: af_unix: Implement " Kuniyuki Iwashima
2021-08-12 16:45 ` [PATCH v5 bpf-next 2/4] bpf: Support "%c" in bpf_bprintf_prepare() Kuniyuki Iwashima
2021-08-12 16:45 ` [PATCH v5 bpf-next 3/4] selftest/bpf: Implement sample UNIX domain socket iterator program Kuniyuki Iwashima
2021-08-13 23:25   ` Andrii Nakryiko
2021-08-14  0:21     ` Kuniyuki Iwashima
2021-08-14  0:26       ` Andrii Nakryiko
2021-08-14  1:00         ` Kuniyuki Iwashima
2021-08-15 18:10       ` Yonghong Song
2021-08-16 12:45         ` Kuniyuki Iwashima
2021-08-12 16:45 ` [PATCH v5 bpf-next 4/4] selftest/bpf: Extend the bpf_snprintf() test for "%c" Kuniyuki Iwashima
2021-08-13 23:28   ` Andrii Nakryiko
2021-08-13 23:30     ` Andrii Nakryiko
2021-08-14  0:37       ` Kuniyuki Iwashima [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210814003702.35395-1-kuniyu@amazon.co.jp \
    --to=kuniyu@amazon.co.jp \
    --cc=andrii.nakryiko@gmail.com \
    --cc=andrii@kernel.org \
    --cc=ast@kernel.org \
    --cc=benh@amazon.com \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=davem@davemloft.net \
    --cc=john.fastabend@gmail.com \
    --cc=kafai@fb.com \
    --cc=kpsingh@kernel.org \
    --cc=kuba@kernel.org \
    --cc=kuni1840@gmail.com \
    --cc=netdev@vger.kernel.org \
    --cc=songliubraving@fb.com \
    --cc=yhs@fb.com \
    --subject='Re: [PATCH v5 bpf-next 4/4] selftest/bpf: Extend the bpf_snprintf() test for "%c".' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).