On 01/04/2018 09:21 AM, Eric Leblond wrote:
> Parse netlink ext attribute to get the error message returned by
> the card. Code is partially take from libnl.
>
> Signed-off-by: Eric Leblond <e...@regit.org>
> Acked-by: Alexei Starovoitov <a...@kernel.org>
> ---
> samples/bpf/Makefile | 2 +-
> tools/lib/bpf/Build | 2 +-
> tools/lib/bpf/bpf.c | 10 ++-
> tools/lib/bpf/nlattr.c | 187
> +++++++++++++++++++++++++++++++++++++++++++++++++
> tools/lib/bpf/nlattr.h | 70 ++++++++++++++++++
> 5 files changed, 268 insertions(+), 3 deletions(-)
> create mode 100644 tools/lib/bpf/nlattr.c
> create mode 100644 tools/lib/bpf/nlattr.h
>
> diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
> index 4fb944a7ecf8..c889ebcba9b3 100644
> --- a/samples/bpf/Makefile
> +++ b/samples/bpf/Makefile
> @@ -44,7 +44,7 @@ hostprogs-y += xdp_monitor
> hostprogs-y += syscall_tp
>
> # Libbpf dependencies
> -LIBBPF := ../../tools/lib/bpf/bpf.o
> +LIBBPF := ../../tools/lib/bpf/bpf.o ../../tools/lib/bpf/nlattr.o
> CGROUP_HELPERS := ../../tools/testing/selftests/bpf/cgroup_helpers.o
>
> test_lru_dist-objs := test_lru_dist.o $(LIBBPF)
> diff --git a/tools/lib/bpf/Build b/tools/lib/bpf/Build
> index d8749756352d..64c679d67109 100644
> --- a/tools/lib/bpf/Build
> +++ b/tools/lib/bpf/Build
> @@ -1 +1 @@
> -libbpf-y := libbpf.o bpf.o
> +libbpf-y := libbpf.o bpf.o nlattr.o
> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> index e6c61035b64c..10d71b9fdbd0 100644
> --- a/tools/lib/bpf/bpf.c
> +++ b/tools/lib/bpf/bpf.c
> @@ -26,6 +26,7 @@
> #include <linux/bpf.h>
> #include "bpf.h"
> #include "libbpf.h"
> +#include "nlattr.h"
> #include <linux/rtnetlink.h>
> #include <sys/socket.h>
> #include <errno.h>
> @@ -440,6 +441,7 @@ int bpf_set_link_xdp_fd(int ifindex, int fd, __u32 flags)
> struct nlmsghdr *nh;
> struct nlmsgerr *err;
> socklen_t addrlen;
> + int one;
Hmm, it's not initialized here to 1 ...
> memset(&sa, 0, sizeof(sa));
> sa.nl_family = AF_NETLINK;
> @@ -449,6 +451,11 @@ int bpf_set_link_xdp_fd(int ifindex, int fd, __u32 flags)
> return -errno;
> }
>
> + if (setsockopt(sock, SOL_NETLINK, NETLINK_EXT_ACK,
> + &one, sizeof(one)) < 0) {
... so we turn it on by chance here.
> + fprintf(stderr, "Netlink error reporting not supported\n");
> + }
> +
> if (bind(sock, (struct sockaddr *)&sa, sizeof(sa)) < 0) {
> ret = -errno;
> goto cleanup;
> @@ -524,7 +531,8 @@ int bpf_set_link_xdp_fd(int ifindex, int fd, __u32 flags)
> err = (struct nlmsgerr *)NLMSG_DATA(nh);
> if (!err->error)
> continue;
> - ret = err->error;
> + ret = -err->error;
This one looks strange. Your prior patch added the 'ret = err->error'
and this one negates it. Which variant is the correct version? From
digging into the kernel code, my take is that 'ret = err->error' was
the correct variant since it already holds the negative error code.
Could you double check?
> + nla_dump_errormsg(nh);
> goto cleanup;
> case NLMSG_DONE:
> break;
Thanks,
Daniel
