From mboxrd@z Thu Jan  1 00:00:00 1970
From: David Ahern <dsahern@gmail.com>
Subject: Re: [PATCH iproute2-next] ip: Add support for nexthop objects
Date: Tue, 4 Sep 2018 09:30:56 -0600
Message-ID: <f6eefabb-fc06-67ce-1e3b-68aab82ff06e@gmail.com>
References: <20180901004954.7145-1-dsahern@kernel.org>
 <20180901004954.7145-20-dsahern@kernel.org> <20180901133753.57f96edd@xeon-e3>
Mime-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Cc: netdev@vger.kernel.org, roopa@cumulusnetworks.com,
        sharpd@cumulusnetworks.com, idosch@mellanox.com,
        davem@davemloft.net
To: Stephen Hemminger <stephen@networkplumber.org>, dsahern@kernel.org
Return-path: <netdev-owner@vger.kernel.org>
Received: from mail-pg1-f195.google.com ([209.85.215.195]:41037 "EHLO
        mail-pg1-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1726377AbeIDT4g (ORCPT
        <rfc822;netdev@vger.kernel.org>); Tue, 4 Sep 2018 15:56:36 -0400
Received: by mail-pg1-f195.google.com with SMTP id s15-v6so1840054pgv.8
        for <netdev@vger.kernel.org>; Tue, 04 Sep 2018 08:30:59 -0700 (PDT)
In-Reply-To: <20180901133753.57f96edd@xeon-e3>
Content-Language: en-US
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

On 9/1/18 2:37 PM, Stephen Hemminger wrote:

>> 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?

I want consistency with rtmsg on which nhmsg is based and has many
parallels.

> 
>> +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.

The file will eventually come from the kernel via header sync, so I have
to stick to whatever style is appropriate for the uapi files.


>> 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@cumulusnetworks.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.

yes, the file pre-dates SPDX. Need to do the same with the kernel side
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?

no. need to fix. The iproute2 patch was only added for the RFC so people
could try out the UAPI which is the point of the RFC.


>> +	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?

oversight. group type was a recent change.