netdev - Re: [PATCH bpf-next] libbpf: make libbpf.map source of truth for libbpf version

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAEf4Bza3xU-Lmgp75CcqEi=-dUeCcfNxi7x8XVE8Gf8xd8=OHQ@mail.gmail.com>
Date:   Wed, 14 Aug 2019 10:28:24 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Andrey Ignatov <rdna@...com>
Cc:     Andrii Nakryiko <andriin@...com>,
        "bpf@...r.kernel.org" <bpf@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...com>,
        "daniel@...earbox.net" <daniel@...earbox.net>,
        Kernel Team <Kernel-team@...com>
Subject: Re: [PATCH bpf-next] libbpf: make libbpf.map source of truth for
 libbpf version

On Wed, Aug 14, 2019 at 12:12 AM Andrey Ignatov <rdna@...com> wrote:
>
> Andrii Nakryiko <andrii.nakryiko@...il.com> [Tue, 2019-08-13 21:46 -0700]:
> > On Tue, Aug 13, 2019 at 5:28 PM Andrey Ignatov <rdna@...com> wrote:
> > >
> > > Andrii Nakryiko <andriin@...com> [Tue, 2019-08-13 16:24 -0700]:
> > > > Currently libbpf version is specified in 2 places: libbpf.map and
> > > > Makefile. They easily get out of sync and it's very easy to update one,
> > > > but forget to update another one. In addition, Github projection of
> > > > libbpf has to maintain its own version which has to be remembered to be
> > > > kept in sync manually, which is very error-prone approach.
> > > >
> > > > This patch makes libbpf.map a source of truth for libbpf version and
> > > > uses shell invocation to parse out correct full and major libbpf version
> > > > to use during build. Now we need to make sure that once new release
> > > > cycle starts, we need to add (initially) empty section to libbpf.map
> > > > with correct latest version.
> > > >
> > > > This also will make it possible to keep Github projection consistent
> > > > with kernel sources version of libbpf by adopting similar parsing of
> > > > version from libbpf.map.
> > >
> > > Thanks for taking care of this!
> > >
> > >
> > > > Cc: Andrey Ignatov <rdna@...com>
> > > > Signed-off-by: Andrii Nakryiko <andriin@...com>
> > > > ---
> > > >  tools/lib/bpf/Makefile   | 12 +++++-------
> > > >  tools/lib/bpf/libbpf.map |  3 +++
> > > >  2 files changed, 8 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile
> > > > index 9312066a1ae3..d9afc8509725 100644
> > > > --- a/tools/lib/bpf/Makefile
> > > > +++ b/tools/lib/bpf/Makefile
> > > > @@ -1,9 +1,10 @@
> > > >  # SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause)
> > > >  # Most of this file is copied from tools/lib/traceevent/Makefile
> > > >
> > > > -BPF_VERSION = 0
> > > > -BPF_PATCHLEVEL = 0
> > > > -BPF_EXTRAVERSION = 4
> > > > +BPF_FULL_VERSION = $(shell \
> > >
> > > Nit: Should it be LIBBPF_VERSION? IMO it's more descriptive name.
> >
> > LIBBPF_VERSION is used below, but combining your suggestion with
> > Jakub's eager evaluation, I can use just LIBBPF_VERSION and drop
> > BPF_FULL_VERSION altogether.
> >
> > >
> > > > +     grep -E 'LIBBPF_([0-9]+)\.([0-9]+)\.([0-9]+) \{' libbpf.map | \
> > > > +     tail -n1 | cut -d'_' -f2 | cut -d' ' -f1)
> > >
> > > It can be done simpler and IMO versions should be sorted before taking
> > > the last one (just in case), something like:
> > >
> > > grep -oE '^LIBBPF_[0-9.]+' libbpf.map | cut -d_ -f 2 | sort -nr | head -n 1
> >
> > Ah, you mean making regex simpler? Yeah, I originally intended to
> > extract major, patch, and extra version, but ralized patch and extra
> > are not used for anything. I'll simplify regex. But second `cut -d' '
> > -f1` is still needed to drop " {".
>
> Yeah, regex, but not only. Note `-o' in the `grep' arguments, it returns
> only matched piece of a string and the second `cut' is not needed.

Oh, TIL, will do -o as well, didn't notice it first time.
>
>
> > Regarding sorting. I don't think it's necessary, as I can't imagine
> > having non-ordered libbpf.map. Even more so, sort -nr doesn't sort
> > versions like these correctly anyway:
> >
> > 0.1.2
> > 0.1.12
> >
> > So this will just give us false sense of correctness, while being a "time bomb".
>
> Right, `-n' is not a good one, `-V' is much better since it's intended
> to sort specifically versions:
>
>   % printf "0.1.2\n0.1.12\n0.1.11\n"
>   0.1.2
>   0.1.12
>   0.1.11
>   % printf "0.1.2\n0.1.12\n0.1.11\n" | sort -cV
>   sort: -:3: disorder: 0.1.11
>   % printf "0.1.2\n0.1.12\n0.1.11\n" | sort -V
>   0.1.2
>   0.1.11
>   0.1.12
>
>
> The reason I brought this up is the version string can be an arbitrary string
> and for example glibc does this:
>
>   % grep -Eo '^\s+GLIBC_\S+' sysdeps/unix/sysv/linux/Versions | tail -n 3
>   GLIBC_2.29
>   GLIBC_2.30
>   GLIBC_PRIVATE
>
> I agree though that it's not a problem with the current version script
> structure and it should be fine to postpone adding some kind of sorting till
> the time this structure is changed (if at all).

I like sort -V, will use that in v3, thanks!

>
> > > > +BPF_VERSION = $(firstword $(subst ., ,$(BPF_FULL_VERSION)))
> > > >
> > > >  MAKEFLAGS += --no-print-directory
> > > >
> > > > @@ -79,15 +80,12 @@ export prefix libdir src obj
> > > >  libdir_SQ = $(subst ','\'',$(libdir))
> > > >  libdir_relative_SQ = $(subst ','\'',$(libdir_relative))
> > > >
> > > > +LIBBPF_VERSION       = $(BPF_FULL_VERSION)
> > > >  VERSION              = $(BPF_VERSION)
> > > > -PATCHLEVEL   = $(BPF_PATCHLEVEL)
> > > > -EXTRAVERSION = $(BPF_EXTRAVERSION)
> > > >
> > > >  OBJ          = $@
> > > >  N            =
> > > >
> > > > -LIBBPF_VERSION       = $(BPF_VERSION).$(BPF_PATCHLEVEL).$(BPF_EXTRAVERSION)
> > > > -
> > > >  LIB_TARGET   = libbpf.a libbpf.so.$(LIBBPF_VERSION)
> > > >  LIB_FILE     = libbpf.a libbpf.so*
> > > >  PC_FILE              = libbpf.pc
> > > > diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> > > > index f9d316e873d8..4e72df8e98ba 100644
> > > > --- a/tools/lib/bpf/libbpf.map
> > > > +++ b/tools/lib/bpf/libbpf.map
> > > > @@ -184,3 +184,6 @@ LIBBPF_0.0.4 {
> > > >               perf_buffer__new_raw;
> > > >               perf_buffer__poll;
> > > >  } LIBBPF_0.0.3;
> > > > +
> > > > +LIBBPF_0.0.5 {
> > > > +} LIBBPF_0.0.4;
> > >
> > > I'm not sure version should be bumped in this patch since this patch is
> > > about keeping the version in one place, not about bumping it, right?
> >
> > This is actually fixing a version. Current libbpf version in bpf-next
> > is 0.0.5, it just was never updated in Makefile.
> >
> > >
> > >
> > > > --
> > > > 2.17.1
> > > >
> > >
> > > --
> > > Andrey Ignatov
>
> --
> Andrey Ignatov