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