* Re: contributions to iproute2
[not found] ` <20150320160819.1477223d@uryu.home.lan>
@ 2015-03-21 4:14 ` Joe Harvell
2015-03-21 6:25 ` Vadim Kochan
2015-03-21 4:23 ` contributions to iproute2 (resend with patches fixed) Joe Harvell
1 sibling, 1 reply; 3+ messages in thread
From: Joe Harvell @ 2015-03-21 4:14 UTC (permalink / raw)
To: netdev; +Cc: Stephen Hemminger
[-- Attachment #1: Type: text/plain, Size: 3548 bytes --]
Thanks, Stephen.
The bugfix is attached as fix-broken-get_prefix_1.diff with the
following commit log:
commit 415464c94a62cfaa9c5ba493e45ce24a58d2118a
Author: Joe Harvell <joe.harvell@tekcomms.com>
Date: Fri Mar 20 15:08:51 2015 -0500
Fixing obvious error of passing in the wrong variable for the
family parameter
of af_bit_len.
I assume master must have some new change because this fix was needed
for a basic 'ip addr add 10.0.3.1/24 dev dumbo label foo' command I
pased in. In this case, 'family' passed into get_addr_1 two lines above
is zero, causing get_addr_1 to detect the family from the address and
populate the result in the family field in dst. But then instead of
passing in the result, family (still 0) is passed in to af_bit_len.
Without my change, the above command complains that 10.0.3.1/24 is not
an address prefix. With the change it works fine as expected.
The enhancement is attached as addr-label-noncompat.diff with the
following commit log which speaks for itself.
commit 51b21cc0382e8a99246289a13e6e842f10e23c80
Author: Joe Harvell <joe.harvell@tekcomms.com>
Date: Fri Mar 20 15:02:16 2015 -0500
Making -force option of ip command also allow address labels that
are not
backward-compatible with ifconfig. Note that even without this change
the ip command does allow some incompatible address labels to be
created.
ifconfig depends on the labels beginning with
<interface-name><colon>, but
the ip command (even before the changes in this commit) only
requires the
prefix of the label to be <interface-name>. Thus, if you add a
label such
as eth0-media, it will be accepted by ip but ifconfig will barf on
ifconfig -a.
The motivation for this change is that the lenght allowed for a
lable is small,
so requiring a long prefix for ifconfig backwards compatibility
limits the
usefulness of the label. For embedded systems (or any system)
where ifconfig
is not even installed, it is useful to be able to create longer
labels.
The only thing I have to add to the above is that maybe also the
existing check (when -force is not specified) should be strengthened to
reject any labels not beginning with <interface-name><colon> instead of
just <interface-name>. Try the following command sequence to the
existing code to see what I mean:
sudo ip link add name dumbo type dummy
sudo ip addr add 10.0.1.0/24 dev dumbo label dumbo-a
ifconfig -a
You will see ifconfig abort when it encouters the address label dumo-a
that it cannot map back to an interface. I think the intent of the
existing check is to prevent people from inadvertently creating such a
configuration. I think -force is a good option to enable it since
anyone enabling this option must realize it and are not doing it
inadvertently.
Thanks for your consideration.
---
Joe Harvell
On 20/03/2015 16:08, Stephen Hemminger wrote:
> On Fri, 20 Mar 2015 20:17:26 +0000
> Joe Harvell <jharvell@dogpad.tk> wrote:
>
>> Hi Stephen,
>>
>> I have an obvious bugfix and small enhancement to the ip command. I
>> have each implemented in a separate branch baselined from today's master
>> (git://git.kernel.org/pub/scm/linux/kernel/git/shemminger/iproute2.git)
>> and would like to submit the changes for consideration to be merged to
>> master. I cannot push these branches, so what is the procedure?
>>
>> Thanks.
>>
>> ---
>> Joe Harvell
>>
> Send patch to netdev@vger.kernel.org
> It will get reviewed there and I will track it in patchwork
[-- Attachment #2: fix-broken-get_prefix_1.diff --]
[-- Type: text/plain, Size: 378 bytes --]
diff --git a/lib/utils.c b/lib/utils.c
index 0d08a86..9cda268 100644
--- a/lib/utils.c
+++ b/lib/utils.c
@@ -477,7 +477,7 @@ int get_prefix_1(inet_prefix *dst, char *arg, int family)
err = get_addr_1(dst, arg, family);
if (err == 0) {
- dst->bitlen = af_bit_len(dst->family);
+ dst->bitlen = af_bit_len(family);
if (slash) {
if (get_netmask(&plen, slash+1, 0)
[-- Attachment #3: addr-label-noncompat.diff --]
[-- Type: text/plain, Size: 1851 bytes --]
diff --git a/include/utils.h b/include/utils.h
index e8a5467..9151c4f 100644
--- a/include/utils.h
+++ b/include/utils.h
@@ -25,7 +25,6 @@ extern char * _SL_;
extern int max_flush_loops;
extern int batch_mode;
extern bool do_all;
-extern bool require_ifconfig_compat;
#ifndef IPPROTO_ESP
#define IPPROTO_ESP 50
diff --git a/ip/ip.c b/ip/ip.c
index 26f9910..da16b15 100644
--- a/ip/ip.c
+++ b/ip/ip.c
@@ -37,7 +37,6 @@ int force = 0;
int max_flush_loops = 10;
int batch_mode = 0;
bool do_all = false;
-bool require_ifconfig_compat = true;
struct rtnl_handle rth = { .fd = -1 };
@@ -247,7 +246,6 @@ int main(int argc, char **argv)
exit(0);
} else if (matches(opt, "-force") == 0) {
++force;
- require_ifconfig_compat = false;
} else if (matches(opt, "-batch") == 0) {
argc--;
argv++;
diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index a1fa785..99a6ab5 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -1691,7 +1691,7 @@ static int ipaddr_modify(int cmd, int flags, int argc, char **argv)
fprintf(stderr, "Not enough information: \"dev\" argument is required.\n");
return -1;
}
- if (require_ifconfig_compat && l && matches(d, l) != 0) {
+ if (l && matches(d, l) != 0) {
fprintf(stderr, "\"dev\" (%s) must match \"label\" (%s).\n", d, l);
return -1;
}
diff --git a/man/man8/ip.8 b/man/man8/ip.8
index 3c3512c..016e8c6 100644
--- a/man/man8/ip.8
+++ b/man/man8/ip.8
@@ -54,8 +54,6 @@ First failure will cause termination of ip.
Don't terminate ip on errors in batch mode.
If there were any errors during execution of the commands, the application return code will be non zero.
-This option also allows creation of address labels that may not be backwards compatible with ifconfig.
-
.TP
.BR "\-s" , " \-stats" , " \-statistics"
Output more information. If the option
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: contributions to iproute2 (resend with patches fixed)
[not found] ` <20150320160819.1477223d@uryu.home.lan>
2015-03-21 4:14 ` contributions to iproute2 Joe Harvell
@ 2015-03-21 4:23 ` Joe Harvell
1 sibling, 0 replies; 3+ messages in thread
From: Joe Harvell @ 2015-03-21 4:23 UTC (permalink / raw)
To: netdev; +Cc: Stephen Hemminger
[-- Attachment #1: Type: text/plain, Size: 3759 bytes --]
Sorry I should have proof-read this before sending. The patches in the
previous email were reversed. These are correct.
------------------------------------------------------------------------
Thanks, Stephen.
The bugfix is attached as fix-broken-get_prefix_1.diff with the
following commit log:
commit 415464c94a62cfaa9c5ba493e45ce24a58d2118a
Author: Joe Harvell <joe.harvell@tekcomms.com>
Date: Fri Mar 20 15:08:51 2015 -0500
Fixing obvious error of passing in the wrong variable for the
family parameter
of af_bit_len.
I assume master must have some new change because this fix was needed
for a basic 'ip addr add 10.0.3.1/24 dev dumbo label foo' command I
pased in. In this case, 'family' passed into get_addr_1 two lines above
is zero, causing get_addr_1 to detect the family from the address and
populate the result in the family field in dst. But then instead of
passing in the result, family (still 0) is passed in to af_bit_len.
Without my change, the above command complains that 10.0.3.1/24 is not
an address prefix. With the change it works fine as expected.
The enhancement is attached as addr-label-noncompat.diff with the
following commit log which speaks for itself.
commit 51b21cc0382e8a99246289a13e6e842f10e23c80
Author: Joe Harvell <joe.harvell@tekcomms.com>
Date: Fri Mar 20 15:02:16 2015 -0500
Making -force option of ip command also allow address labels that
are not
backward-compatible with ifconfig. Note that even without this
change
the ip command does allow some incompatible address labels to be
created.
ifconfig depends on the labels beginning with
<interface-name><colon>, but
the ip command (even before the changes in this commit) only
requires the
prefix of the label to be <interface-name>. Thus, if you add a
label such
as eth0-media, it will be accepted by ip but ifconfig will barf
on ifconfig -a.
The motivation for this change is that the lenght allowed for a
lable is small,
so requiring a long prefix for ifconfig backwards compatibility
limits the
usefulness of the label. For embedded systems (or any system)
where ifconfig
is not even installed, it is useful to be able to create longer
labels.
The only thing I have to add to the above is that maybe also the
existing check (when -force is not specified) should be strengthened to
reject any labels not beginning with <interface-name><colon> instead of
just <interface-name>. Try the following command sequence to the
existing code to see what I mean:
sudo ip link add name dumbo type dummy
sudo ip addr add 10.0.1.0/24 dev dumbo label dumbo-a
ifconfig -a
You will see ifconfig abort when it encouters the address label dumo-a
that it cannot map back to an interface. I think the intent of the
existing check is to prevent people from inadvertently creating such a
configuration. I think -force is a good option to enable it since
anyone enabling this option must realize it and are not doing it
inadvertently.
Thanks for your consideration.
---
Joe Harvell
On 20/03/2015 16:08, Stephen Hemminger wrote:
> On Fri, 20 Mar 2015 20:17:26 +0000
> Joe Harvell <jharvell@dogpad.tk> wrote:
>
>> Hi Stephen,
>>
>> I have an obvious bugfix and small enhancement to the ip command. I
>> have each implemented in a separate branch baselined from today's master
>> (git://git.kernel.org/pub/scm/linux/kernel/git/shemminger/iproute2.git)
>> and would like to submit the changes for consideration to be merged to
>> master. I cannot push these branches, so what is the procedure?
>>
>> Thanks.
>>
>> ---
>> Joe Harvell
>>
> Send patch to netdev@vger.kernel.org
> It will get reviewed there and I will track it in patchwork
[-- Attachment #2: fix-broken-get_prefix_1.diff --]
[-- Type: text/plain, Size: 377 bytes --]
diff --git a/lib/utils.c b/lib/utils.c
index 9cda268..0d08a86 100644
--- a/lib/utils.c
+++ b/lib/utils.c
@@ -477,7 +477,7 @@ int get_prefix_1(inet_prefix *dst, char *arg, int family)
err = get_addr_1(dst, arg, family);
if (err == 0) {
- dst->bitlen = af_bit_len(family);
+ dst->bitlen = af_bit_len(dst->family);
if (slash) {
if (get_netmask(&plen, slash+1, 0)
[-- Attachment #3: addr-label-noncompat.diff --]
[-- Type: text/plain, Size: 1850 bytes --]
diff --git a/include/utils.h b/include/utils.h
index 9151c4f..e8a5467 100644
--- a/include/utils.h
+++ b/include/utils.h
@@ -25,6 +25,7 @@ extern char * _SL_;
extern int max_flush_loops;
extern int batch_mode;
extern bool do_all;
+extern bool require_ifconfig_compat;
#ifndef IPPROTO_ESP
#define IPPROTO_ESP 50
diff --git a/ip/ip.c b/ip/ip.c
index da16b15..26f9910 100644
--- a/ip/ip.c
+++ b/ip/ip.c
@@ -37,6 +37,7 @@ int force = 0;
int max_flush_loops = 10;
int batch_mode = 0;
bool do_all = false;
+bool require_ifconfig_compat = true;
struct rtnl_handle rth = { .fd = -1 };
@@ -246,6 +247,7 @@ int main(int argc, char **argv)
exit(0);
} else if (matches(opt, "-force") == 0) {
++force;
+ require_ifconfig_compat = false;
} else if (matches(opt, "-batch") == 0) {
argc--;
argv++;
diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index 99a6ab5..a1fa785 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -1691,7 +1691,7 @@ static int ipaddr_modify(int cmd, int flags, int argc, char **argv)
fprintf(stderr, "Not enough information: \"dev\" argument is required.\n");
return -1;
}
- if (l && matches(d, l) != 0) {
+ if (require_ifconfig_compat && l && matches(d, l) != 0) {
fprintf(stderr, "\"dev\" (%s) must match \"label\" (%s).\n", d, l);
return -1;
}
diff --git a/man/man8/ip.8 b/man/man8/ip.8
index 016e8c6..3c3512c 100644
--- a/man/man8/ip.8
+++ b/man/man8/ip.8
@@ -54,6 +54,8 @@ First failure will cause termination of ip.
Don't terminate ip on errors in batch mode.
If there were any errors during execution of the commands, the application return code will be non zero.
+This option also allows creation of address labels that may not be backwards compatible with ifconfig.
+
.TP
.BR "\-s" , " \-stats" , " \-statistics"
Output more information. If the option
^ permalink raw reply related [flat|nested] 3+ messages in thread