On Wed, 23 Jan 2019 14:41:59 +0100
Maciej Fijalkowski [off-list ref] wrote:

On Wed, 23 Jan 2019 11:41:11 +0100
Daniel Borkmann [off-list ref] wrote:

quoted

On 01/21/2019 10:10 AM, Maciej Fijalkowski wrote:  

quoted

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;
}