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