Re: [PATCH iproute2-next v2 3/7] iplink: Use "dev" and "name" parameters interchangeable when possible

[Serhey Popovych]

Stephen Hemminger wrote:
> On Tue, 20 Feb 2018 23:37:25 +0200
> Serhey Popovych  wrote:
> 
>> Both of them accept network device name as argument, but have different
>> meaning:
>>
>>   dev  - is a device by it's name,
>>   name - name for specific device device.
>>
>> The only case where they treated separately is network device rename
>> case where need to specify both ifindex and new name. In rest of the
>> cases we can assume that dev == name.
> 
> I would rather keep name as only being a valid argument for set command.
> And reject use of:
> 
>ip li show name eth0
> 

But ip-link(8) man page is full of examples where "name" is used instead
of "dev". I suppose there are lot of configuration we can broke when
explicitly disabling "name" as an alternative to "dev".

Especially on the first lines in man page we have:

ip link add [ link DEVICE ] [ name ] NAME
...

and there is no reference to "dev".

What do you think?



signature.asc
Description: OpenPGP digital signature

Re: [PATCH iproute2-next v2 3/7] iplink: Use "dev" and "name" parameters interchangeable when possible

[Stephen Hemminger]

On Tue, 20 Feb 2018 23:37:25 +0200
Serhey Popovych  wrote:

> Both of them accept network device name as argument, but have different
> meaning:
> 
>   dev  - is a device by it's name,
>   name - name for specific device device.
> 
> The only case where they treated separately is network device rename
> case where need to specify both ifindex and new name. In rest of the
> cases we can assume that dev == name.

I would rather keep name as only being a valid argument for set command.
And reject use of:

   ip li show name eth0

[PATCH iproute2-next v2 3/7] iplink: Use "dev" and "name" parameters interchangeable when possible

[Serhey Popovych]

Both of them accept network device name as argument, but have different
meaning:

  dev  - is a device by it's name,
  name - name for specific device device.

The only case where they treated separately is network device rename
case where need to specify both ifindex and new name. In rest of the
cases we can assume that dev == name.

With this change we do following:

  1) Kill ambiguity with both "dev" and "name" parameters given the same
 name:

   ip link {add|set} dev veth100a name veth100a ...

  2) Make sure we do not accept "name" more than once.

  3) For VF and XDP treat "name" as "dev". Fail in case of "dev" is
 given after VF and/or XDP parsing.

  4) Make veth and vxcan to accept both "name" and "dev" as their peer
 parameters, effectively following general ip-link(8) utility
 behaviour on link create:

   ip link add {name|dev} veth1a type veth peer {name|dev} veth1b

Signed-off-by: Serhey Popovych 
---
 ip/iplink.c |   34 ++
 1 file changed, 30 insertions(+), 4 deletions(-)

diff --git a/ip/iplink.c b/ip/iplink.c
index fc358fc..1359c0f 100644
--- a/ip/iplink.c
+++ b/ip/iplink.c
@@ -605,9 +605,15 @@ int iplink_parse(int argc, char **argv, struct iplink_req 
*req,
req->i.ifi_flags &= ~IFF_UP;
} else if (strcmp(*argv, "name") == 0) {
NEXT_ARG();
+   if (*name)
+   duparg("name", *argv);
if (check_ifname(*argv))
invarg("\"name\" not a valid ifname", *argv);
*name = *argv;
+   if (!*dev) {
+   *dev = *name;
+   dev_index = ll_name_to_index(*dev);
+   }
} else if (strcmp(*argv, "index") == 0) {
NEXT_ARG();
if (*index)
@@ -665,6 +671,9 @@ int iplink_parse(int argc, char **argv, struct iplink_req 
*req,
if (xdp_parse(&argc, &argv, req, dev_index,
  generic, drv, offload))
exit(-1);
+
+   if (offload && *name == *dev)
+   *dev = NULL;
} else if (strcmp(*argv, "netns") == 0) {
NEXT_ARG();
if (netns != -1)
@@ -755,6 +764,9 @@ int iplink_parse(int argc, char **argv, struct iplink_req 
*req,
if (len < 0)
return -1;
addattr_nest_end(&req->n, vflist);
+
+   if (*name == *dev)
+   *dev = NULL;
} else if (matches(*argv, "master") == 0) {
int ifindex;
 
@@ -905,7 +917,7 @@ int iplink_parse(int argc, char **argv, struct iplink_req 
*req,
 
if (strcmp(*argv, "dev") == 0)
NEXT_ARG();
-   if (*dev)
+   if (*dev != *name)
duparg2("dev", *argv);
if (check_ifname(*argv))
invarg("\"dev\" not a valid ifname", *argv);
@@ -915,6 +927,14 @@ int iplink_parse(int argc, char **argv, struct iplink_req 
*req,
argc--; argv++;
}
 
+   /* Allow "ip link add dev" and "ip link add name" */
+   if (!*name)
+   *name = *dev;
+   else if (!*dev)
+   *dev = *name;
+   else if (!strcmp(*name, *dev))
+   *name = *dev;
+
if (dev_index && addr_len) {
int halen = nl_get_ll_addr_len(dev_index);
 
@@ -993,10 +1013,16 @@ static int iplink_modify(int cmd, unsigned int flags, 
int argc, char **argv)
req.i.ifi_index = ll_name_to_index(dev);
if (!req.i.ifi_index)
return nodev(dev);
+
+   /* Not renaming to the same name */
+   if (name == dev)
+   name = NULL;
} else {
-   /* Allow "ip link add dev" and "ip link add name" */
-   if (!name)
-   name = dev;
+   if (name != dev) {
+   fprintf(stderr,
+   "both \"name\" and \"dev\" cannot be used when 
creating devices.\n");
+   exit(-1);
+   }
 
if (link) {
int ifindex;
-- 
1.7.10.4