Date:   Sat, 1 Sep 2018 13:37:53 -0700
From:   Stephen Hemminger <stephen@...workplumber.org>
To:     dsahern@...nel.org
Cc:     netdev@...r.kernel.org, roopa@...ulusnetworks.com,
        sharpd@...ulusnetworks.com, idosch@...lanox.com,
        davem@...emloft.net, David Ahern <dsahern@...il.com>
Subject: Re: [PATCH iproute2-next] ip: Add support for nexthop objects

On Fri, 31 Aug 2018 17:49:54 -0700
dsahern@...nel.org wrote:

> From: David Ahern <dsahern@...il.com>
> 
> Signed-off-by: David Ahern <dsahern@...il.com>
> ---
>  include/uapi/linux/nexthop.h   |  56 ++++
>  include/uapi/linux/rtnetlink.h |   8 +
>  ip/Makefile                    |   3 +-
>  ip/ip.c                        |   3 +-
>  ip/ip_common.h                 |   7 +-
>  ip/ipmonitor.c                 |   6 +
>  ip/ipnexthop.c                 | 652 +++++++++++++++++++++++++++++++++++++++++
>  ip/iproute.c                   |  19 +-
>  8 files changed, 747 insertions(+), 7 deletions(-)
>  create mode 100644 include/uapi/linux/nexthop.h
>  create mode 100644 ip/ipnexthop.c
> 
> diff --git a/include/uapi/linux/nexthop.h b/include/uapi/linux/nexthop.h
> new file mode 100644
> index 000000000000..335182e8229a
> --- /dev/null
> +++ b/include/uapi/linux/nexthop.h
> @@ -0,0 +1,56 @@
> +#ifndef __LINUX_NEXTHOP_H
> +#define __LINUX_NEXTHOP_H
> +
> +#include <linux/types.h>
> +
> +struct nhmsg {
> +	unsigned char	nh_family;
> +	unsigned char	nh_scope;     /* one of RT_SCOPE */
> +	unsigned char	nh_protocol;  /* Routing protocol that installed nh */
> +	unsigned char	resvd;
> +	unsigned int	nh_flags;     /* RTNH_F flags */
> +};

Why not use __u8 and __u32 for these?

> +struct nexthop_grp {
> +	__u32	id;
> +	__u32	weight;
> +};
> +
> +enum {
> +	NEXTHOP_GRP_TYPE_MPATH,  /* default type if not specified */
> +	__NEXTHOP_GRP_TYPE_MAX,
> +};
> +
> +#define NEXTHOP_GRP_TYPE_MAX (__NEXTHOP_GRP_TYPE_MAX - 1)
> +
> +
> +/* NHA_ID	32-bit id for nexthop. id must be greater than 0.
> + *		id == 0 means assign an unused id.
> + */

Don't use dave's preferred comment style in this file.
The reset of the file uses standard comments.
...

> diff --git a/ip/ip_common.h b/ip/ip_common.h
> index 200be5e23dd1..2971c1586c4e 100644
> --- a/ip/ip_common.h
> +++ b/ip/ip_common.h
> @@ -56,6 +56,8 @@ int print_rule(const struct sockaddr_nl *who,
>  int print_netconf(const struct sockaddr_nl *who,
>  		  struct rtnl_ctrl_data *ctrl,
>  		  struct nlmsghdr *n, void *arg);
> +int print_nexthop(const struct sockaddr_nl *who,
> +		  struct nlmsghdr *n, void *arg);
>  void netns_map_init(void);
>  void netns_nsid_socket_init(void);
>  int print_nsid(const struct sockaddr_nl *who,
> @@ -90,6 +92,7 @@ int do_ipvrf(int argc, char **argv);
>  void vrf_reset(void);
>  int netns_identify_pid(const char *pidstr, char *name, int len);
>  int do_seg6(int argc, char **argv);
> +int do_ipnh(int argc, char **argv);
>  
>  int iplink_get(unsigned int flags, char *name, __u32 filt_mask);
>  int iplink_ifla_xstats(int argc, char **argv);
> @@ -165,5 +168,7 @@ int name_is_vrf(const char *name);
>  #endif
>  
>  void print_num(FILE *fp, unsigned int width, uint64_t count);
> -
> +void print_rta_flow(FILE *fp, const struct rtattr *rta);
> +void print_rt_flags(FILE *fp, unsigned int flags);
> +void print_rta_if(FILE *fp, const struct rtattr *rta, const char *prefix);
>  #endif /* _IP_COMMON_H_ */
> diff --git a/ip/ipmonitor.c b/ip/ipmonitor.c
> index a93b62cd6624..de129626683b 100644
> --- a/ip/ipmonitor.c
> +++ b/ip/ipmonitor.c
> @@ -84,6 +84,12 @@ static int accept_msg(const struct sockaddr_nl *who,
>  		}
>  	}
>  
> +	case RTM_NEWNEXTHOP:
> +	case RTM_DELNEXTHOP:
> +		print_headers(fp, "[NEXTHOP]", ctrl);
> +		print_nexthop(who, n, arg);
> +		return 0;
> +
>  	case RTM_NEWLINK:
>  	case RTM_DELLINK:
>  		ll_remember_index(who, n, NULL);
> diff --git a/ip/ipnexthop.c b/ip/ipnexthop.c
> new file mode 100644
> index 000000000000..9fa4b7292426
> --- /dev/null
> +++ b/ip/ipnexthop.c
> @@ -0,0 +1,652 @@
> +/*
> + * ip nexthop
> + *
> + * Copyright (C) 2017 Cumulus Networks
> + * Copyright (c) 2017 David Ahern <dsa@...ulusnetworks.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
>

Please use SPDX and not GPL boilerplate in new files.

> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include <string.h>
> +#include <sys/socket.h>
> +#include <netinet/in.h>
> +#include <netinet/ip.h>
> +#include <errno.h>
> +#include <linux/nexthop.h>
> +#include <libmnl/libmnl.h>

Is this code really using libmnl?

> +#include <rt_names.h>
> +
> +#include "utils.h"
> +#include "ip_common.h"
> +
> +static struct
> +{
> +	unsigned int flushed;
> +	unsigned int groups;
> +	char *flushb;
> +	int flushp;
> +	int flushe;
> +} filter;
> +
> +enum {
> +	IPNH_LIST,
> +	IPNH_FLUSH,
> +};
> +
> +#define RTM_NHA(h)  ((struct rtattr *)(((char *)(h)) + \
> +			NLMSG_ALIGN(sizeof(struct nhmsg))))
> +
> +static void usage(void) __attribute__((noreturn));
> +
> +static void usage(void)
> +{
> +	fprintf(stderr, "Usage: ip nexthop { list | flush } SELECTOR\n");
> +	fprintf(stderr, "       ip nexthop get [ id ID ]\n");
> +	fprintf(stderr, "       ip nexthop { add | del | change | replace } NH\n");
> +	fprintf(stderr, "SELECTOR := [ id ID ] [ dev DEV ] [ table TABLE ] [ vrf NAME ]\n");
> +	fprintf(stderr, "NH := [ encap ENCAPTYPE ENCAPHDR ] [ via [ FAMILY ] ADDRESS ]\n");
> +	fprintf(stderr, "      [ id ID ] [ dev STRING ] [ weight NUMBER ]\n");
> +	fprintf(stderr, "      [ table TABLE ] [ vrf VRF ] NHFLAGS\n");
> +	fprintf(stderr, "NHFLAGS := [ onlink | pervasive ]\n");
> +	fprintf(stderr, "ENCAPTYPE := [ mpls | ip | ip6 ]\n");
> +	fprintf(stderr, "ENCAPHDR := [ MPLSLABEL ]\n");
> +	exit(-1);
> +}
> +
> +static int delete_nexthop(__u32 id)
> +{
> +	struct {
> +		struct nlmsghdr	n;
> +		struct nhmsg	nhm;
> +		char		buf[64];
> +	} req = {
> +		.n.nlmsg_len = NLMSG_LENGTH(sizeof(struct nhmsg)),
> +		.n.nlmsg_flags = NLM_F_REQUEST,
> +		.n.nlmsg_type = RTM_DELNEXTHOP,
> +		.nhm.nh_family = AF_UNSPEC,
> +	};
> +
> +	req.n.nlmsg_seq = ++rth.seq;
> +
> +	addattr32(&req.n, sizeof(req), NHA_ID, id);
> +
> +	if (rtnl_talk(&rth, &req.n, NULL) < 0)
> +		return -1;
> +
> +	filter.flushed++;
> +	return 0;
> +}
> +
> +struct nh_entry {
> +	__u32 id;
> +	unsigned int group;
> +	struct nh_entry *next;
> +};
> +
> +struct nh_entry *first, *last;
> +
> +static int flush_nexthop(const struct sockaddr_nl *who,
> +			 struct nlmsghdr *nlh, void *arg)
> +{
> +	struct nhmsg *nhm = NLMSG_DATA(nlh);
> +	struct rtattr *tb[NHA_MAX+1];
> +	struct nh_entry *nh;
> +	__u32 id = 0;
> +	int len;
> +
> +	len = nlh->nlmsg_len - NLMSG_SPACE(sizeof(*nhm));
> +	if (len < 0) {
> +		fprintf(stderr, "BUG: wrong nlmsg len %d\n", len);
> +		return -1;
> +	}
> +
> +	parse_rtattr(tb, NHA_MAX, RTM_NHA(nhm), len);
> +	if (tb[NHA_ID])
> +		id = rta_getattr_u32(tb[NHA_ID]);
> +
> +	if (!id)
> +		return 0;
> +
> +	nh = malloc(sizeof(*nh));
> +	if (!nh)
> +		return -1;
> +
> +	nh->id = id;
> +	nh->group = tb[NHA_GROUP] != NULL;
> +	nh->next = NULL;
> +	if (!first)
> +		first = nh;
> +	else
> +		last->next = nh;
> +
> +	last = nh;
> +	return 0;
> +}
> +
> +static int ipnh_flush(void *req, __u32 len, unsigned int all)
> +{
> +	struct nh_entry *nh;
> +
> +	if (send(rth.fd, req, len, 0) < 0) {
> +		perror("Cannot send dump request");
> +		return -2;
> +	}
> +
> +	if (rtnl_dump_filter(&rth, flush_nexthop, stdout) < 0) {
> +		fprintf(stderr, "Dump terminated\n");
> +		return -2;
> +	}
> +
> +	/* if deleting all, then remove groups first */
> +	if (all) {
> +		nh = first;
> +		while (nh) {
> +			if (nh->group)
> +				delete_nexthop(nh->id);
> +			nh = nh->next;
> +		}
> +	}
> +
> +	nh = first;
> +	while (nh) {
> +		if (!all || !nh->group)
> +			delete_nexthop(nh->id);
> +		nh = nh->next;
> +	}
> +
> +	if (!filter.flushed)
> +		printf("Nothing to flush\n");
> +	else
> +		printf("Flushed %d nexthops\n", filter.flushed);
> +
> +	return 0;
> +}
> +
> +static char *nh_group_type_to_str(__u16 group_type, char *buf, size_t len)
> +{
> +	static const char *typestr[NEXTHOP_GRP_TYPE_MAX + 1] = {
> +		"multipath",   /* NEXTHOP_GRP_TYPE_MPATH */
> +	};
> +
> +	if (group_type < ARRAY_SIZE(typestr))
> +		snprintf(buf, len-1, "%s", typestr[group_type]);
> +	else
> +		snprintf(buf, len-1, "<%u>", group_type);
> +
> +	buf[len-1] = '\0';
> +
> +	return buf;
> +}
> +
> +static void print_nh_group(FILE *fp, const struct rtattr *grps_attr,
> +			   const struct rtattr *gtype)
> +{
> +	struct nexthop_grp *nhg = RTA_DATA(grps_attr);
> +	int num = RTA_PAYLOAD(grps_attr) / sizeof(*nhg);
> +	__u16 group_type = NEXTHOP_GRP_TYPE_MPATH;
> +	int i;
> +
> +	SPRINT_BUF(b1);
> +
> +	if (!num || num * sizeof(*nhg) != RTA_PAYLOAD(grps_attr)) {
> +		fprintf(fp, "<invalid nexthop group>");
> +		return;
> +	}
> +
> +	if (gtype)
> +		group_type = rta_getattr_u16(gtype);
> +
> +	if (is_json_context()) {
> +		open_json_array(PRINT_JSON, "group");
> +		for (i = 0; i < num; ++i) {
> +			open_json_object(NULL);
> +			print_uint(PRINT_ANY, "id", "id %u ", nhg[i].id);
> +			print_uint(PRINT_ANY, "weight", "weight %u ", nhg[i].weight);
> +			close_json_object();
> +		}
> +		close_json_array(PRINT_JSON, NULL);
> +		print_string(PRINT_ANY, "type", "type %s ",
> +			     nh_group_type_to_str(group_type, b1, sizeof(b1)));
> +	} else {
> +		fprintf(fp, "group ");
> +		for (i = 0; i < num; ++i) {
> +			if (i)
> +				fprintf(fp, "/");
> +			fprintf(fp, "%u", nhg[i].id);
> +			if (num > 1 && nhg[i].weight > 1)
> +				fprintf(fp, ",%u", nhg[i].weight);
> +		}
> +	}
> +}

I think this could be done by using json_print cleverly rather than having
to use is_json_contex(). That would avoid repeating code.

You are only decoding group type in the json version, why not both?

Powered by blists - more mailing lists