On Wed, Jul 28, 2021 at 2:54 PM Quentin Monnet [off-list ref] wrote:

On Tue, 27 Jul 2021 at 21:49, Andrii Nakryiko [off-list ref] wrote:

quoted

quoted

quoted

quoted

quoted

LIBBPF_DEPRECATED_SINCE(0, 6, "API that will be marked deprecated in v0.6")

So I've been looking into this, and it's not _that_ simple to do. Unless
I missed something about preprocessing macros, I cannot bake a "#if" in
a "#define", to have the attribute printed if and only if the current
version is >= 0.6 in this example.

I've come up with something, but it is not optimal because I have to
write a check and macros for each version number used with the
LIBBPF_DEPRECATED_SINCE macro. If we really wanted to automate that part
I guess we could generate a header with those macros from the Makefile
and include it in libbpf_common.h, but that does not really look much
cleaner to me.

Yeah, let's not add unnecessary code generation. It sucks, of course,
that we can't do #ifdef inside a macro :(

So it's either do something like what you did with defining
version-specific macros, which is actually not too bad, because it's
not like we have tons of those versions anyways.

LIBBPF_DEPRECATED_SINCE(0, 6, "use btf__load_from_kernel_by_id instead")
LIBBPF_API int btf__get_from_id(__u32 id, struct btf **btf);

Alternatively, we can go with:

#if LIBBPF_AT_OR_NEWER(0, 6)
LIBBPF_DEPRECATED("use btf__load_from_kernel_by_id instead")
#endif
LIBBPF API int btf__get_from_id(__u32 id, struct btf **btf);

I don't really dislike the second variant too much either, but
LIBBPF_DEPRECATED_SINCE() reads nicer. Let's go with that. See some
comments below about implementation.

Ok.

quoted

quoted

Here's my current code, below - does it correspond to what you had in
mind? Or did you think of something else?

------

diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile
index ec14aa725bb0..095d5dc30d50 100644
--- a/tools/lib/bpf/Makefile
+++ b/tools/lib/bpf/Makefile

@@ -8,6 +8,7 @@ LIBBPF_VERSION := $(shell \
        grep -oE '^LIBBPF_([0-9.]+)' libbpf.map | \
        sort -rV | head -n1 | cut -d'_' -f2)
 LIBBPF_MAJOR_VERSION := $(firstword $(subst ., ,$(LIBBPF_VERSION)))
+LIBBPF_MINOR_VERSION := $(firstword $(subst ., ,$(subst $(LIBBPF_MAJOR_VERSION)., ,$(LIBBPF_VERSION))))

Given all this is for internal use, I'd instead define something like
__LIBBPF_CURVER as an integer that is easy to compare against:

#define __LIBBPF_CURVER (LIBBPF_MAJOR_VERSION * 100 +
LIBBPF_MINOR_VERSION) * 100 + LIBBPF_PATCH_VERSION

That will simplify some stuff below and is generally easier to use in
code, if we will need this somewhere to use explicitly.

Did you mean computing __LIBBPF_CURVER in the Makefile, or in the
header?

I was thinking Makefile, but if it's simpler to do in the header
that's fine as well.

I can do that if you want, although I'm not convinced it will simplify
much. Instead of having one long-ish condition, we'll have to compute
the integer for the current version, as well as for each of the versions
that we list for deprecating functions. I suppose I can add another
dedicated macro.

feels like if we need to do some comparisons, then writing

#if __LIBBPF_VER > 102
/* do something */
#endif

is much simpler than comparing MAJOR_VERSION and MINOR_VERSION
separately. It's just that currently with 0 major version it might
look a bit awkward right now, but that's temporary.

Do you actually want the patch version? I chose to leave it aside
because 1) I thought it would not be relevant for deprecating symbols,
and 2) if anything like a -rc1 suffix is ever appended to the version,
it makes it more complex to parse from the version string.

yeah, you are probably right. major  and minor should be enough

quoted

quoted

 MAKEFLAGS += --no-print-directory

@@ -86,6 +87,8 @@ override CFLAGS += -Werror -Wall
 override CFLAGS += $(INCLUDES)
 override CFLAGS += -fvisibility=hidden
 override CFLAGS += -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64
+override CFLAGS += -DLIBBPF_MAJOR_VERSION=$(LIBBPF_MAJOR_VERSION)
+override CFLAGS += -DLIBBPF_MINOR_VERSION=$(LIBBPF_MINOR_VERSION)

 # flags specific for shared library
 SHLIB_FLAGS := -DSHARED -fPIC

diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
index cf8490f95641..8b6b5442dbd8 100644
--- a/tools/lib/bpf/btf.h
+++ b/tools/lib/bpf/btf.h

@@ -45,7 +45,8 @@ LIBBPF_API struct btf *btf__parse_raw(const char *path);
 LIBBPF_API struct btf *btf__parse_raw_split(const char *path, struct btf *base_btf);
 LIBBPF_API struct btf *btf__load_from_kernel_by_id(__u32 id);
 LIBBPF_API struct btf *btf__load_from_kernel_by_id_split(__u32 id, struct btf *base_btf);
-LIBBPF_API int btf__get_from_id(__u32 id, struct btf **btf);
+LIBBPF_API LIBBPF_DEPRECATED_SINCE(0, 6, "use btf__load_from_kernel_by_id instead")

nit: given how long those deprecations will be, let's keep them at a
separate (first) line and keep LIBBPF_API near the function
declaration itself

I thought having the LIBBPF_API on a separate line would slightly reduce
the risk, when moving lines around, to move the function prototype but
not the deprecation attribute. But ok, fine.

highly improbable and then we'll most probably catch it during build anyways

quoted

quoted

+int btf__get_from_id(__u32 id, struct btf **btf);

 LIBBPF_API int btf__finalize_data(struct bpf_object *obj, struct btf *btf);
 LIBBPF_API int btf__load(struct btf *btf);

diff --git a/tools/lib/bpf/libbpf_common.h b/tools/lib/bpf/libbpf_common.h
index 947d8bd8a7bb..9ba9f8135dc8 100644
--- a/tools/lib/bpf/libbpf_common.h
+++ b/tools/lib/bpf/libbpf_common.h

@@ -17,6 +17,28 @@

 #define LIBBPF_DEPRECATED(msg) __attribute__((deprecated(msg)))

+#ifndef LIBBPF_DEPRECATED_SINCE

why #ifndef conditional?

Right, we don't expect to have the macro defined elsewhere. I'll remove
it.

quoted

quoted

+#define __LIBBPF_VERSION_CHECK(major, minor) \
+       LIBBPF_MAJOR_VERSION > major || \
+               (LIBBPF_MAJOR_VERSION == major && LIBBPF_MINOR_VERSION >= minor)

so we don't need this if we do __LIBBPF_CURVER

Right, but we do need to compute an integer for each of the versions
listed below (0.6 for now). I'll see if I can come up with something
short.

see above, I'd just do 102 etc. I wonder if 006 will be treated as an
octal number, in that case probably fine to do just 6. Or we can have
a small macro for this, of course. Don't know, doesn't seem to matter
all that much

quoted

quoted

+
+/* Add checks for other versions below when planning deprecation of API symbols
+ * with the LIBBPF_DEPRECATED_SINCE macro.
+ */
+#if __LIBBPF_VERSION_CHECK(0, 6)
+#define __LIBBPF_MARK_DEPRECATED_0_6(X) X
+#else
+#define __LIBBPF_MARK_DEPRECATED_0_6(X)
+#endif
+
+#define __LIBBPF_DEPRECATED_SINCE(major, minor, msg) \
+       __LIBBPF_MARK_DEPRECATED_ ## major ## _ ## minor (LIBBPF_DEPRECATED("v" # major "." # minor "+, " msg))
+
+/* Mark a symbol as deprecated when libbpf version is >= {major}.{minor} */
+#define LIBBPF_DEPRECATED_SINCE(major, minor, msg) \
+       __LIBBPF_DEPRECATED_SINCE(major, minor, msg)

Is it needed for some macro value concatenation magic to have this
nested __LIBBPF_DEPRECATED_SINCE?

I double-checked (I needed to, anyway), and it seems not. It's a
leftover from an earlier version of my code, I'll clean it up before
the proper submission.

ok, thanks

Thanks!
Quentin