Re: contributions to iproute2

Re: contributions to iproute2

[Joe Harvell]

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

Re: contributions to iproute2 (resend with patches fixed)

[Joe Harvell]

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

Re: contributions to iproute2

[Vadim Kochan]

On Fri, Mar 20, 2015 at 11:14:04PM -0500, Joe Harvell wrote:
> 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.
> 
Hi,

Thanks for catching it, this is a serious issue.

Does not the diff should be ?

-               dst->bitlen = af_bit_len(family);
+               dst->bitlen = af_bit_len(dst->family);

Also you can use 'git send-email' to send separate patchs as email,
instead of attaching them.

Regards,