LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Jakub Kicinski <jakub.kicinski@netronome.com>
To: Krzesimir Nowak <krzesimir@kinvolk.io>
Cc: bpf@vger.kernel.org, "Iago López Galeiras" <iago@kinvolk.io>,
	"Alban Crequy (Kinvolk)" <alban@kinvolk.io>,
	"Shuah Khan" <shuah@kernel.org>,
	"Alexei Starovoitov" <ast@kernel.org>,
	"Daniel Borkmann" <daniel@iogearbox.net>,
	"Martin KaFai Lau" <kafai@fb.com>,
	"Song Liu" <songliubraving@fb.com>, "Yonghong Song" <yhs@fb.com>,
	"Andrey Ignatov" <rdna@fb.com>,
	linux-kselftest@vger.kernel.org, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org, ogerlitz@mellanox.com
Subject: Re: [PATCH bpf v1 2/3] selftests/bpf: Print a message when tester could not run a program
Date: Thu, 16 May 2019 09:54:26 -0700	[thread overview]
Message-ID: <20190516095426.2e0d838f@cakuba.netronome.com> (raw)
In-Reply-To: <CAGGp+cEFxzbH-8vnSAK3sZkM-u3WN4HGnkYvhFwBp85yVtD7Xg@mail.gmail.com>

On Thu, 16 May 2019 18:21:32 +0200, Krzesimir Nowak wrote:
> On Thu, May 16, 2019 at 5:51 PM Jakub Kicinski wrote:
> > On Thu, 16 May 2019 11:29:39 +0200, Krzesimir Nowak wrote:  
> > > > > diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> > > > > index ccd896b98cac..bf0da03f593b 100644
> > > > > --- a/tools/testing/selftests/bpf/test_verifier.c
> > > > > +++ b/tools/testing/selftests/bpf/test_verifier.c
> > > > > @@ -825,11 +825,20 @@ static int do_prog_test_run(int fd_prog, bool unpriv, uint32_t expected_val,
> > > > >                               tmp, &size_tmp, &retval, NULL);
> > > > >       if (unpriv)
> > > > >               set_admin(false);
> > > > > -     if (err && errno != 524/*ENOTSUPP*/ && errno != EPERM) {
> > > > > -             printf("Unexpected bpf_prog_test_run error ");
> > > > > -             return err;
> > > > > +     if (err) {
> > > > > +             switch (errno) {
> > > > > +             case 524/*ENOTSUPP*/:
> > > > > +                     printf("Did not run the program (not supported) ");
> > > > > +                     return 0;
> > > > > +             case EPERM:
> > > > > +                     printf("Did not run the program (no permission) ");
> > > > > +                     return 0;  
> > > >
> > > > Perhaps use strerror(errno)?  
> > >
> > > As I said in the commit message, I open-coded those messages because
> > > strerror for ENOTSUPP returns "Unknown error 524".  
> >
> > Ah, sorry, missed that.  I wonder if that's something worth addressing
> > in libc, since the BPF subsystem uses ENOTSUPP a lot.  
> 
> The "not supported" errno situation seems to be a mess. There is an
> ENOTSUP define in libc. ENOTSUP is usually defined to be EOPNOTSUPP
> (taken from kernel), which in turn seems to have a different value
> (95) than kernel's ENOTSUPP (524). Adding ENOTSUPP (with two Ps) to
> libc would only add to the confusion. So it's kind of meh and I guess
> people just moved on with workarounds.

Yes, ENOTSUP is never used in the kernel, but it's a mess.

This commit a while ago said ENOTSUPP is from NFS:

commit 423b3aecf29085a52530d4f9167c56a84b081042
Author: Or Gerlitz <ogerlitz@mellanox.com>
Date:   Thu Feb 23 12:02:41 2017 +0200

    net/mlx4: Change ENOTSUPP to EOPNOTSUPP
    
    As ENOTSUPP is specific to NFS, change the return error value to
    EOPNOTSUPP in various places in the mlx4 driver.
    
    Signed-off-by: Or Gerlitz <ogerlitz@mellanox.com>
    Suggested-by: Yotam Gigi <yotamg@mellanox.com>
    Reviewed-by: Matan Barak <matanb@mellanox.com>
    Signed-off-by: Tariq Toukan <tariqt@mellanox.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>

But it's spreading throughout the kernel like a wildfire, I counted 1364
in my tree :/  Some are in tools/, but still.  My understanding was that
system calls should never return values above 512, but I'm probably
wrong about that.

Given the popularity, and the fact its an ABI at this point, we
probably have no choice but to add it to libc, but to be clear IMO it's
not a blocker for your patches.

  reply	other threads:[~2019-05-16 16:54 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-15 13:47 [PATCH bpf v1 0/3] Test the 32bit narrow reads Krzesimir Nowak
2019-05-15 13:47 ` [PATCH bpf v1 1/3] selftests/bpf: Test correctness of narrow 32bit read on 64bit field Krzesimir Nowak
2019-05-23 16:02   ` Daniel Borkmann
2019-05-15 13:47 ` [PATCH bpf v1 2/3] selftests/bpf: Print a message when tester could not run a program Krzesimir Nowak
2019-05-15 21:45   ` Jakub Kicinski
2019-05-16  9:29     ` Krzesimir Nowak
2019-05-16 15:50       ` Jakub Kicinski
2019-05-16 16:21         ` Krzesimir Nowak
2019-05-16 16:54           ` Jakub Kicinski [this message]
2019-05-15 13:47 ` [PATCH bpf v1 3/3] selftests/bpf: Avoid a clobbering of errno Krzesimir Nowak
2019-05-15 21:50   ` Jakub Kicinski
2019-05-16  9:31     ` Krzesimir Nowak

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=20190516095426.2e0d838f@cakuba.netronome.com \
    --to=jakub.kicinski@netronome.com \
    --cc=alban@kinvolk.io \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=iago@kinvolk.io \
    --cc=kafai@fb.com \
    --cc=krzesimir@kinvolk.io \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=ogerlitz@mellanox.com \
    --cc=rdna@fb.com \
    --cc=shuah@kernel.org \
    --cc=songliubraving@fb.com \
    --cc=yhs@fb.com \
    --subject='Re: [PATCH bpf v1 2/3] selftests/bpf: Print a message when tester could not run a program' \
    /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).