[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20190123152451.7fcdb226@redhat.com>
Date: Wed, 23 Jan 2019 15:24:51 +0100
From: Jesper Dangaard Brouer <brouer@...hat.com>
To: Maciej Fijalkowski <maciejromanfijalkowski@...il.com>
Cc: Daniel Borkmann <daniel@...earbox.net>, ast@...nel.org,
netdev@...r.kernel.org, jakub.kicinski@...ronome.com,
brouer@...hat.com
Subject: Re: [PATCH bpf-next v2 2/8] libbpf: Add a helper for retrieving a
prog via index
On Wed, 23 Jan 2019 14:41:59 +0100
Maciej Fijalkowski <maciejromanfijalkowski@...il.com> wrote:
> On Wed, 23 Jan 2019 11:41:11 +0100
> Daniel Borkmann <daniel@...earbox.net> wrote:
>
> > On 01/21/2019 10:10 AM, Maciej Fijalkowski wrote:
> > > xdp_redirect_cpu has a 6 different XDP programs that can be attached to
> > > network interface. This sample has a option --prognum that allows user
> > > for specifying which particular program from a given set will be
> > > attached to network interface.
> > > In order to make it easier when converting the mentioned sample to
> > > libbpf usage, add a function to libbpf that will return program's fd for
> > > a given index.
> > >
> > > Note that there is already a bpf_object__find_prog_by_idx, which could
> > > be exported and might be used for that purpose, but it operates on the
> > > number of ELF section and here we need an index from a programs array
> > > within the bpf_object.
> >
> > Series in general looks good to me. Few minor comments, mainly in relation
> > to the need for libbpf extensions.
> >
> > Would it not be a better interface to the user to instead choose the prog
> > based on section name and then retrieve it via bpf_object__find_program_by_title()
> > instead of prognum (which feels less user friendly) at least?
> >
>
> I couldn't decide which one from:
> * adding a libbpf helper
> * changing the xdp_redirect_cpu behaviour
>
> would be more invasive when I was converting this sample to libbpf
> support.
>
> Your suggestion sounds good, but I'm wondering about the actual
> implementation. I suppose that we would choose the program via
> command line. Some program section names in this sample are a bit
> long and it might be irritating for user to type in for example
> "xdp_cpu_map5_lb_hash_ip_pairs", no? Or maybe we can live with this.
> In case of typo and program being not found it would be good to print
> out the section names to choose from in usage().
Please feel free to deprecate or remove the xdp_redirect_cpu --prognum
option. I would prefer if this was indeed converted to selecting the
program based on the name in the _kern.c file, instead of a number.
Listing the avail programs will be helpful, and you can also
shorten/rename the program names.
For easy of use, consider doing like 'ip' tool from[1] iproute2, and
find the first best matching string. Copy pasted code below signature
for inspiration.
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
[1] git://git.kernel.org/pub/scm/network/iproute2/iproute2.git
lib/utils.c
int matches(const char *cmd, const char *pattern)
{
int len = strlen(cmd);
if (len > strlen(pattern))
return -1;
return memcmp(pattern, cmd, len);
}
ip/ip.c
static const struct cmd {
const char *cmd;
int (*func)(int argc, char **argv);
} cmds[] = {
{ "address", do_ipaddr },
{ "addrlabel", do_ipaddrlabel },
{ "maddress", do_multiaddr },
{ "route", do_iproute },
{ "rule", do_iprule },
{ "neighbor", do_ipneigh },
{ "neighbour", do_ipneigh },
{ "ntable", do_ipntable },
{ "ntbl", do_ipntable },
{ "link", do_iplink },
{ "l2tp", do_ipl2tp },
{ "fou", do_ipfou },
{ "ila", do_ipila },
{ "macsec", do_ipmacsec },
{ "tunnel", do_iptunnel },
{ "tunl", do_iptunnel },
{ "tuntap", do_iptuntap },
{ "tap", do_iptuntap },
{ "token", do_iptoken },
{ "tcpmetrics", do_tcp_metrics },
{ "tcp_metrics", do_tcp_metrics },
{ "monitor", do_ipmonitor },
{ "xfrm", do_xfrm },
{ "mroute", do_multiroute },
{ "mrule", do_multirule },
{ "netns", do_netns },
{ "netconf", do_ipnetconf },
{ "vrf", do_ipvrf},
{ "sr", do_seg6 },
{ "help", do_help },
{ 0 }
};
static int do_cmd(const char *argv0, int argc, char **argv)
{
const struct cmd *c;
for (c = cmds; c->cmd; ++c) {
if (matches(argv0, c->cmd) == 0)
return -(c->func(argc-1, argv+1));
}
fprintf(stderr, "Object \"%s\" is unknown, try \"ip help\".\n", argv0);
return EXIT_FAILURE;
}
Powered by blists - more mailing lists