diff mbox series

[v2,2/2] bpf: fix convert API can be undefined

Message ID 20211101161013.31966-3-konstantin.ananyev@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series few bpf library fixes |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-aarch64-unit-testing success Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/github-robot: build success github build: passed
ci/intel-Testing success Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-intel-Performance success Performance Testing PASS

Commit Message

Ananyev, Konstantin Nov. 1, 2021, 4:10 p.m. UTC 
  rte_bpf_convert() implementation depends on libpcap.
Right now it is defined only when this library is installed and
RTE_PORT_PCAP is defined.
Fix that by providing for such case stub rte_bpf_convert()
implementation that will always return an error.
Also move stub for another function (rte_bpf_elf_load) into
the same place (bpf_stub.c).

Fixes: 2eccf6afbea9 ("bpf: add function to convert classic BPF to DPDK BPF")

Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
---
 lib/bpf/bpf_load.c  | 18 ------------------
 lib/bpf/bpf_stub.c  | 45 +++++++++++++++++++++++++++++++++++++++++++++
 lib/bpf/meson.build |  1 +
 lib/bpf/rte_bpf.h   |  5 +----
 4 files changed, 47 insertions(+), 22 deletions(-)
 create mode 100644 lib/bpf/bpf_stub.c

Comments

Stephen Hemminger Nov. 1, 2021, 5:05 p.m. UTC | #1 
On Mon,  1 Nov 2021 16:10:13 +0000
Konstantin Ananyev <konstantin.ananyev@intel.com> wrote:

> rte_bpf_convert() implementation depends on libpcap.
> Right now it is defined only when this library is installed and
> RTE_PORT_PCAP is defined.
> Fix that by providing for such case stub rte_bpf_convert()
> implementation that will always return an error.
> Also move stub for another function (rte_bpf_elf_load) into
> the same place (bpf_stub.c).
> 
> Fixes: 2eccf6afbea9 ("bpf: add function to convert classic BPF to DPDK BPF")
> 
> Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

Wouldn't it be better to fail compiling a program using unimplemented calls
rather than forcing the user to see a failure later?
Ananyev, Konstantin Nov. 2, 2021, 10:54 a.m. UTC | #2 
> > rte_bpf_convert() implementation depends on libpcap.
> > Right now it is defined only when this library is installed and
> > RTE_PORT_PCAP is defined.
> > Fix that by providing for such case stub rte_bpf_convert()
> > implementation that will always return an error.
> > Also move stub for another function (rte_bpf_elf_load) into
> > the same place (bpf_stub.c).
> >
> > Fixes: 2eccf6afbea9 ("bpf: add function to convert classic BPF to DPDK BPF")
> >
> > Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> 
> Wouldn't it be better to fail compiling a program using unimplemented calls
> rather than forcing the user to see a failure later?

You mean to keep things as they are right now?
That way we'll have to put #ifdef RTE_PORT_PCAP around every call to bpf_convert().
Doesn't look like a good thing to me.
Also in other places we use similar approach: if HW/SW is not available provide
a dummy implementation that will report an error.
Stephen Hemminger Nov. 2, 2021, 3:05 p.m. UTC | #3 
On Tue, 2 Nov 2021 10:54:59 +0000
"Ananyev, Konstantin" <konstantin.ananyev@intel.com> wrote:

> > > rte_bpf_convert() implementation depends on libpcap.
> > > Right now it is defined only when this library is installed and
> > > RTE_PORT_PCAP is defined.
> > > Fix that by providing for such case stub rte_bpf_convert()
> > > implementation that will always return an error.
> > > Also move stub for another function (rte_bpf_elf_load) into
> > > the same place (bpf_stub.c).
> > >
> > > Fixes: 2eccf6afbea9 ("bpf: add function to convert classic BPF to DPDK BPF")
> > >
> > > Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>  
> > 
> > Wouldn't it be better to fail compiling a program using unimplemented calls
> > rather than forcing the user to see a failure later?  
> 
> You mean to keep things as they are right now?
> That way we'll have to put #ifdef RTE_PORT_PCAP around every call to bpf_convert().
> Doesn't look like a good thing to me.
> Also in other places we use similar approach: if HW/SW is not available provide
> a dummy implementation that will report an error. 

Is there a way to do both, return an error and report a warning in the build?
Stephen Hemminger Nov. 2, 2021, 3:27 p.m. UTC | #4 
On Mon,  1 Nov 2021 16:10:13 +0000
Konstantin Ananyev <konstantin.ananyev@intel.com> wrote:

> rte_bpf_convert() implementation depends on libpcap.
> Right now it is defined only when this library is installed and
> RTE_PORT_PCAP is defined.
> Fix that by providing for such case stub rte_bpf_convert()
> implementation that will always return an error.
> Also move stub for another function (rte_bpf_elf_load) into
> the same place (bpf_stub.c).
> 
> Fixes: 2eccf6afbea9 ("bpf: add function to convert classic BPF to DPDK BPF")
> 
> Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

Should also change dumpcap to print more complete error?


diff --git a/app/dumpcap/main.c b/app/dumpcap/main.c
index baf9eee46666..fff460eee518 100644
--- a/app/dumpcap/main.c
+++ b/app/dumpcap/main.c
@@ -285,7 +285,7 @@ static void compile_filter(void)
        bpf_prm = rte_bpf_convert(&bf);
        if (bpf_prm == NULL)
                rte_exit(EXIT_FAILURE,
-                        "bpf convert failed\n");
+                        "bpf convert failed: %s\n", rte_strerror(rte_errno));
 
        if (dump_bpf) {
                printf("cBPF program (%u insns)\n", bf.bf_len);
Ananyev, Konstantin Nov. 3, 2021, 10:54 a.m. UTC | #5 
> On Tue, 2 Nov 2021 10:54:59 +0000
> "Ananyev, Konstantin" <konstantin.ananyev@intel.com> wrote:
> 
> > > > rte_bpf_convert() implementation depends on libpcap.
> > > > Right now it is defined only when this library is installed and
> > > > RTE_PORT_PCAP is defined.
> > > > Fix that by providing for such case stub rte_bpf_convert()
> > > > implementation that will always return an error.
> > > > Also move stub for another function (rte_bpf_elf_load) into
> > > > the same place (bpf_stub.c).
> > > >
> > > > Fixes: 2eccf6afbea9 ("bpf: add function to convert classic BPF to DPDK BPF")
> > > >
> > > > Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> > >
> > > Wouldn't it be better to fail compiling a program using unimplemented calls
> > > rather than forcing the user to see a failure later?
> >
> > You mean to keep things as they are right now?
> > That way we'll have to put #ifdef RTE_PORT_PCAP around every call to bpf_convert().
> > Doesn't look like a good thing to me.
> > Also in other places we use similar approach: if HW/SW is not available provide
> > a dummy implementation that will report an error.
> 
> Is there a way to do both, return an error and report a warning in the build?

Sounds like a good idea, will add a warning to meson.build.
diff mbox series

Patch

diff --git a/lib/bpf/bpf_load.c b/lib/bpf/bpf_load.c
index 2a3b901d74..272f2ba11b 100644
--- a/lib/bpf/bpf_load.c
+++ b/lib/bpf/bpf_load.c
@@ -130,21 +130,3 @@  rte_bpf_load(const struct rte_bpf_prm *prm)
 
 	return bpf;
 }
-
-#ifndef RTE_LIBRTE_BPF_ELF
-struct rte_bpf *
-rte_bpf_elf_load(const struct rte_bpf_prm *prm, const char *fname,
-	const char *sname)
-{
-	if (prm == NULL || fname == NULL || sname == NULL) {
-		rte_errno = EINVAL;
-		return NULL;
-	}
-
-	RTE_BPF_LOG(ERR, "%s() is not supported with current config\n"
-		"rebuild with libelf installed\n",
-		__func__);
-	rte_errno = ENOTSUP;
-	return NULL;
-}
-#endif
diff --git a/lib/bpf/bpf_stub.c b/lib/bpf/bpf_stub.c
new file mode 100644
index 0000000000..caec00f42f
--- /dev/null
+++ b/lib/bpf/bpf_stub.c
@@ -0,0 +1,45 @@ 
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2018-2021 Intel Corporation
+ */
+
+#include "bpf_impl.h"
+#include <rte_errno.h>
+
+/**
+ * Contains stubs for unimplemented public API functions
+ */
+
+#ifndef RTE_LIBRTE_BPF_ELF
+struct rte_bpf *
+rte_bpf_elf_load(const struct rte_bpf_prm *prm, const char *fname,
+	const char *sname)
+{
+	if (prm == NULL || fname == NULL || sname == NULL) {
+		rte_errno = EINVAL;
+		return NULL;
+	}
+
+	RTE_BPF_LOG(ERR, "%s() is not supported with current config\n"
+		"rebuild with libelf installed\n",
+		__func__);
+	rte_errno = ENOTSUP;
+	return NULL;
+}
+#endif
+
+#ifndef RTE_PORT_PCAP
+struct rte_bpf_prm *
+rte_bpf_convert(const struct bpf_program *prog)
+{
+	if (prog == NULL) {
+		rte_errno = EINVAL;
+		return NULL;
+	}
+
+	RTE_BPF_LOG(ERR, "%s() is not supported with current config\n"
+		"rebuild with libpcap installed\n",
+		__func__);
+	rte_errno = ENOTSUP;
+	return NULL;
+}
+#endif
diff --git a/lib/bpf/meson.build b/lib/bpf/meson.build
index 33b506f3ac..0ec067957b 100644
--- a/lib/bpf/meson.build
+++ b/lib/bpf/meson.build
@@ -12,6 +12,7 @@  sources = files('bpf.c',
         'bpf_exec.c',
         'bpf_load.c',
         'bpf_pkt.c',
+        'bpf_stub.c',
         'bpf_validate.c')
 
 if arch_subdir == 'x86' and dpdk_conf.get('RTE_ARCH_64')
diff --git a/lib/bpf/rte_bpf.h b/lib/bpf/rte_bpf.h
index f09e36b65b..7e7d03b9c7 100644
--- a/lib/bpf/rte_bpf.h
+++ b/lib/bpf/rte_bpf.h
@@ -212,8 +212,6 @@  __rte_experimental
 void
 rte_bpf_dump(FILE *f, const struct ebpf_insn *buf, uint32_t len);
 
-#ifdef RTE_PORT_PCAP
-
 struct bpf_program;
 
 /**
@@ -228,13 +226,12 @@  struct bpf_program;
  *   Possible rte_errno errors include:
  *   - EINVAL - invalid parameter passed to function
  *   - ENOMEM - can't reserve enough memory
+ *   - ENOTSUP - operation not supported
  */
 __rte_experimental
 struct rte_bpf_prm *
 rte_bpf_convert(const struct bpf_program *prog);
 
-#endif
-
 #ifdef __cplusplus
 }
 #endif