[iproute PATCH 0/2] Resend: Simplify and enhance vf_info parsing

[iproute PATCH 0/2] Resend: Simplify and enhance vf_info parsing

[Phil Sutter]

This patch series got lost in a discussion about whether the code the
first patch removes is necessary or not - static analysis as well as my
tests showed it is not. Therefore resending this with updated
description of patch 1 to contain the discussion's gist.

Phil Sutter (2):
  ipaddress: Simplify vf_info parsing
  ipaddress: Print IFLA_VF_QUERY_RSS_EN setting

 ip/ipaddress.c | 52 ++++++++++++++++++----------------------------------
 1 file changed, 18 insertions(+), 34 deletions(-)

-- 
2.10.0

[iproute PATCH 1/2] ipaddress: Simplify vf_info parsing

[Phil Sutter]

Commit 7b8179c780a1a ("iproute2: Add new command to ip link to
enable/disable VF spoof check") tried to add support for
IFLA_VF_SPOOFCHK in a backwards-compatible manner, but aparently overdid
it: parse_rtattr_nested() handles missing attributes perfectly fine in
that it will leave the relevant field unassigned so calling code can
just compare against NULL. There is no need to layback from the previous
(IFLA_VF_TX_RATE) attribute to the next to check if IFLA_VF_SPOOFCHK is
present or not. To the contrary, it establishes a potentially incorrect
assumption of these two attributes directly following each other which
may not be the case (although up to now, kernel aligns them this way).

This patch cleans up the code to adhere to the common way of checking
for attribute existence. It has been tested to return correct results
regardless of whether the kernel exports IFLA_VF_SPOOFCHK or not.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 ip/ipaddress.c | 44 ++++++++++----------------------------------
 1 file changed, 10 insertions(+), 34 deletions(-)

diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index 7f05258f43453..df0f1b9c94c58 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -322,10 +322,7 @@ static void print_vfinfo(FILE *fp, struct rtattr *vfinfo)
 {
 	struct ifla_vf_mac *vf_mac;
 	struct ifla_vf_tx_rate *vf_tx_rate;
-	struct ifla_vf_spoofchk *vf_spoofchk;
-	struct ifla_vf_link_state *vf_linkstate;
 	struct rtattr *vf[IFLA_VF_MAX + 1] = {};
-	struct rtattr *tmp;
 
 	SPRINT_BUF(b1);
 
@@ -339,31 +336,6 @@ static void print_vfinfo(FILE *fp, struct rtattr *vfinfo)
 	vf_mac = RTA_DATA(vf[IFLA_VF_MAC]);
 	vf_tx_rate = RTA_DATA(vf[IFLA_VF_TX_RATE]);
 
-	/* Check if the spoof checking vf info type is supported by
-	 * this kernel.
-	 */
-	tmp = (struct rtattr *)((char *)vf[IFLA_VF_TX_RATE] +
-				vf[IFLA_VF_TX_RATE]->rta_len);
-
-	if (tmp->rta_type != IFLA_VF_SPOOFCHK)
-		vf_spoofchk = NULL;
-	else
-		vf_spoofchk = RTA_DATA(vf[IFLA_VF_SPOOFCHK]);
-
-	if (vf_spoofchk) {
-		/* Check if the link state vf info type is supported by
-		 * this kernel.
-		 */
-		tmp = (struct rtattr *)((char *)vf[IFLA_VF_SPOOFCHK] +
-					vf[IFLA_VF_SPOOFCHK]->rta_len);
-
-		if (tmp->rta_type != IFLA_VF_LINK_STATE)
-			vf_linkstate = NULL;
-		else
-			vf_linkstate = RTA_DATA(vf[IFLA_VF_LINK_STATE]);
-	} else
-		vf_linkstate = NULL;
-
 	fprintf(fp, "%s    vf %d MAC %s", _SL_, vf_mac->vf,
 		ll_addr_n2a((unsigned char *)&vf_mac->mac,
 			    ETH_ALEN, 0, b1, sizeof(b1)));
@@ -407,14 +379,18 @@ static void print_vfinfo(FILE *fp, struct rtattr *vfinfo)
 		if (vf_rate->min_tx_rate)
 			fprintf(fp, ", min_tx_rate %dMbps", vf_rate->min_tx_rate);
 	}
+	if (vf[IFLA_VF_SPOOFCHK]) {
+		struct ifla_vf_spoofchk *vf_spoofchk =
+			RTA_DATA(vf[IFLA_VF_SPOOFCHK]);
 
-	if (vf_spoofchk && vf_spoofchk->setting != -1) {
-		if (vf_spoofchk->setting)
-			fprintf(fp, ", spoof checking on");
-		else
-			fprintf(fp, ", spoof checking off");
+		if (vf_spoofchk->setting != -1)
+			fprintf(fp, ", spoof checking %s",
+			        vf_spoofchk->setting ? "on" : "off");
 	}
-	if (vf_linkstate) {
+	if (vf[IFLA_VF_LINK_STATE]) {
+		struct ifla_vf_link_state *vf_linkstate =
+			RTA_DATA(vf[IFLA_VF_LINK_STATE]);
+
 		if (vf_linkstate->link_state == IFLA_VF_LINK_STATE_AUTO)
 			fprintf(fp, ", link-state auto");
 		else if (vf_linkstate->link_state == IFLA_VF_LINK_STATE_ENABLE)
-- 
2.10.0

Re: [iproute PATCH 1/2] ipaddress: Simplify vf_info parsing

[Greg]

On Tue, 2016-11-08 at 22:29 +0100, Phil Sutter wrote:
> Commit 7b8179c780a1a ("iproute2: Add new command to ip link to
> enable/disable VF spoof check") tried to add support for
> IFLA_VF_SPOOFCHK in a backwards-compatible manner, but aparently overdid
> it: parse_rtattr_nested() handles missing attributes perfectly fine in
> that it will leave the relevant field unassigned so calling code can
> just compare against NULL. There is no need to layback from the previous
> (IFLA_VF_TX_RATE) attribute to the next to check if IFLA_VF_SPOOFCHK is
> present or not. To the contrary, it establishes a potentially incorrect
> assumption of these two attributes directly following each other which
> may not be the case (although up to now, kernel aligns them this way).
> 
> This patch cleans up the code to adhere to the common way of checking
> for attribute existence. It has been tested to return correct results
> regardless of whether the kernel exports IFLA_VF_SPOOFCHK or not.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  ip/ipaddress.c | 44 ++++++++++----------------------------------
>  1 file changed, 10 insertions(+), 34 deletions(-)
> 
> diff --git a/ip/ipaddress.c b/ip/ipaddress.c
> index 7f05258f43453..df0f1b9c94c58 100644
> --- a/ip/ipaddress.c
> +++ b/ip/ipaddress.c
> @@ -322,10 +322,7 @@ static void print_vfinfo(FILE *fp, struct rtattr *vfinfo)
>  {
>  	struct ifla_vf_mac *vf_mac;
>  	struct ifla_vf_tx_rate *vf_tx_rate;
> -	struct ifla_vf_spoofchk *vf_spoofchk;
> -	struct ifla_vf_link_state *vf_linkstate;
>  	struct rtattr *vf[IFLA_VF_MAX + 1] = {};
> -	struct rtattr *tmp;
>  
>  	SPRINT_BUF(b1);
>  
> @@ -339,31 +336,6 @@ static void print_vfinfo(FILE *fp, struct rtattr *vfinfo)
>  	vf_mac = RTA_DATA(vf[IFLA_VF_MAC]);
>  	vf_tx_rate = RTA_DATA(vf[IFLA_VF_TX_RATE]);
>  
> -	/* Check if the spoof checking vf info type is supported by
> -	 * this kernel.
> -	 */
> -	tmp = (struct rtattr *)((char *)vf[IFLA_VF_TX_RATE] +
> -				vf[IFLA_VF_TX_RATE]->rta_len);
> -
> -	if (tmp->rta_type != IFLA_VF_SPOOFCHK)
> -		vf_spoofchk = NULL;
> -	else
> -		vf_spoofchk = RTA_DATA(vf[IFLA_VF_SPOOFCHK]);
> -
> -	if (vf_spoofchk) {
> -		/* Check if the link state vf info type is supported by
> -		 * this kernel.
> -		 */
> -		tmp = (struct rtattr *)((char *)vf[IFLA_VF_SPOOFCHK] +
> -					vf[IFLA_VF_SPOOFCHK]->rta_len);
> -
> -		if (tmp->rta_type != IFLA_VF_LINK_STATE)
> -			vf_linkstate = NULL;
> -		else
> -			vf_linkstate = RTA_DATA(vf[IFLA_VF_LINK_STATE]);
> -	} else
> -		vf_linkstate = NULL;
> -
>  	fprintf(fp, "%s    vf %d MAC %s", _SL_, vf_mac->vf,
>  		ll_addr_n2a((unsigned char *)&vf_mac->mac,
>  			    ETH_ALEN, 0, b1, sizeof(b1)));
> @@ -407,14 +379,18 @@ static void print_vfinfo(FILE *fp, struct rtattr *vfinfo)
>  		if (vf_rate->min_tx_rate)
>  			fprintf(fp, ", min_tx_rate %dMbps", vf_rate->min_tx_rate);
>  	}
> +	if (vf[IFLA_VF_SPOOFCHK]) {
> +		struct ifla_vf_spoofchk *vf_spoofchk =
> +			RTA_DATA(vf[IFLA_VF_SPOOFCHK]);
>  
> -	if (vf_spoofchk && vf_spoofchk->setting != -1) {
> -		if (vf_spoofchk->setting)
> -			fprintf(fp, ", spoof checking on");
> -		else
> -			fprintf(fp, ", spoof checking off");
> +		if (vf_spoofchk->setting != -1)
> +			fprintf(fp, ", spoof checking %s",
> +			        vf_spoofchk->setting ? "on" : "off");

I wrote some of this code at a time when I was pretty new to Linux
kernel net programming and I really just didn't understand it.  It
appears you're doing it more correctly than I.

Thanks for cleaning it up.

Reviewed-by: Greg Rose <grose@lightfleet.com>

- Greg

>  	}
> -	if (vf_linkstate) {
> +	if (vf[IFLA_VF_LINK_STATE]) {
> +		struct ifla_vf_link_state *vf_linkstate =
> +			RTA_DATA(vf[IFLA_VF_LINK_STATE]);
> +
>  		if (vf_linkstate->link_state == IFLA_VF_LINK_STATE_AUTO)
>  			fprintf(fp, ", link-state auto");
>  		else if (vf_linkstate->link_state == IFLA_VF_LINK_STATE_ENABLE)

[iproute PATCH 2/2] ipaddress: Print IFLA_VF_QUERY_RSS_EN setting

[Phil Sutter]

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 ip/ipaddress.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index df0f1b9c94c58..c9f769fb748e4 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -405,6 +405,14 @@ static void print_vfinfo(FILE *fp, struct rtattr *vfinfo)
 			fprintf(fp, ", trust %s",
 			        vf_trust->setting ? "on" : "off");
 	}
+	if (vf[IFLA_VF_RSS_QUERY_EN]) {
+		struct ifla_vf_rss_query_en *rss_query =
+			RTA_DATA(vf[IFLA_VF_RSS_QUERY_EN]);
+
+		if (rss_query->setting != -1)
+			fprintf(fp, ", query_rss %s",
+			        rss_query->setting ? "on" : "off");
+	}
 	if (vf[IFLA_VF_STATS] && show_stats)
 		print_vf_stats64(fp, vf[IFLA_VF_STATS]);
 }
-- 
2.10.0

[iproute PATCH 0/2] Minor vf_info review

[Phil Sutter]

While implementing the display of IFLA_VF_QUERY_RSS_EN value in
ip-address (see patch 2/2), I stumbled upon some dubious constructs in
print_vfinfo() and replaced them with code which seems more intuitive to
me (see patch 1/2 for details).

Phil Sutter (2):
  ipaddress: Simplify vf_info parsing
  ipaddress: Print IFLA_VF_QUERY_RSS_EN setting

 ip/ipaddress.c | 52 ++++++++++++++++++----------------------------------
 1 file changed, 18 insertions(+), 34 deletions(-)

-- 
2.8.2

[iproute PATCH 1/2] ipaddress: Simplify vf_info parsing

[Phil Sutter]

Not sure whether I misinterpret commit 7b8179c780a1a, but it looks
overly complicated. Instead rely upon parse_rtattr_nested() to assign
the relevant pointer if requested rtattr fields are present.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 ip/ipaddress.c | 44 ++++++++++----------------------------------
 1 file changed, 10 insertions(+), 34 deletions(-)

diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index df363b070d5de..9ac077459f136 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -305,10 +305,7 @@ static void print_vfinfo(FILE *fp, struct rtattr *vfinfo)
 	struct ifla_vf_mac *vf_mac;
 	struct ifla_vf_vlan *vf_vlan;
 	struct ifla_vf_tx_rate *vf_tx_rate;
-	struct ifla_vf_spoofchk *vf_spoofchk;
-	struct ifla_vf_link_state *vf_linkstate;
 	struct rtattr *vf[IFLA_VF_MAX + 1] = {};
-	struct rtattr *tmp;
 
 	SPRINT_BUF(b1);
 
@@ -323,31 +320,6 @@ static void print_vfinfo(FILE *fp, struct rtattr *vfinfo)
 	vf_vlan = RTA_DATA(vf[IFLA_VF_VLAN]);
 	vf_tx_rate = RTA_DATA(vf[IFLA_VF_TX_RATE]);
 
-	/* Check if the spoof checking vf info type is supported by
-	 * this kernel.
-	 */
-	tmp = (struct rtattr *)((char *)vf[IFLA_VF_TX_RATE] +
-				vf[IFLA_VF_TX_RATE]->rta_len);
-
-	if (tmp->rta_type != IFLA_VF_SPOOFCHK)
-		vf_spoofchk = NULL;
-	else
-		vf_spoofchk = RTA_DATA(vf[IFLA_VF_SPOOFCHK]);
-
-	if (vf_spoofchk) {
-		/* Check if the link state vf info type is supported by
-		 * this kernel.
-		 */
-		tmp = (struct rtattr *)((char *)vf[IFLA_VF_SPOOFCHK] +
-					vf[IFLA_VF_SPOOFCHK]->rta_len);
-
-		if (tmp->rta_type != IFLA_VF_LINK_STATE)
-			vf_linkstate = NULL;
-		else
-			vf_linkstate = RTA_DATA(vf[IFLA_VF_LINK_STATE]);
-	} else
-		vf_linkstate = NULL;
-
 	fprintf(fp, "%s    vf %d MAC %s", _SL_, vf_mac->vf,
 		ll_addr_n2a((unsigned char *)&vf_mac->mac,
 			    ETH_ALEN, 0, b1, sizeof(b1)));
@@ -366,14 +338,18 @@ static void print_vfinfo(FILE *fp, struct rtattr *vfinfo)
 		if (vf_rate->min_tx_rate)
 			fprintf(fp, ", min_tx_rate %dMbps", vf_rate->min_tx_rate);
 	}
+	if (vf[IFLA_VF_SPOOFCHK]) {
+		struct ifla_vf_spoofchk *vf_spoofchk =
+			RTA_DATA(vf[IFLA_VF_SPOOFCHK]);
 
-	if (vf_spoofchk && vf_spoofchk->setting != -1) {
-		if (vf_spoofchk->setting)
-			fprintf(fp, ", spoof checking on");
-		else
-			fprintf(fp, ", spoof checking off");
+		if (vf_spoofchk->setting != -1)
+			fprintf(fp, ", spoof checking %s",
+			        vf_spoofchk->setting ? "on" : "off");
 	}
-	if (vf_linkstate) {
+	if (vf[IFLA_VF_LINK_STATE]) {
+		struct ifla_vf_link_state *vf_linkstate =
+			RTA_DATA(vf[IFLA_VF_LINK_STATE]);
+
 		if (vf_linkstate->link_state == IFLA_VF_LINK_STATE_AUTO)
 			fprintf(fp, ", link-state auto");
 		else if (vf_linkstate->link_state == IFLA_VF_LINK_STATE_ENABLE)
-- 
2.8.2

Re: [iproute PATCH 1/2] ipaddress: Simplify vf_info parsing

[Greg Rose]

On Wed, Jun 1, 2016 at 1:03 PM, Phil Sutter <phil@nwl.cc> wrote:
> Not sure whether I misinterpret commit 7b8179c780a1a, but it looks
> overly complicated. Instead rely upon parse_rtattr_nested() to assign
> the relevant pointer if requested rtattr fields are present.

I'm not sure if newer iproute2 utilities are supposed to work on older
kernels but if it is you may want to check this against a 2.6.32
kernel.

- Greg Rose

>
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  ip/ipaddress.c | 44 ++++++++++----------------------------------
>  1 file changed, 10 insertions(+), 34 deletions(-)
>
> diff --git a/ip/ipaddress.c b/ip/ipaddress.c
> index df363b070d5de..9ac077459f136 100644
> --- a/ip/ipaddress.c
> +++ b/ip/ipaddress.c
> @@ -305,10 +305,7 @@ static void print_vfinfo(FILE *fp, struct rtattr *vfinfo)
>         struct ifla_vf_mac *vf_mac;
>         struct ifla_vf_vlan *vf_vlan;
>         struct ifla_vf_tx_rate *vf_tx_rate;
> -       struct ifla_vf_spoofchk *vf_spoofchk;
> -       struct ifla_vf_link_state *vf_linkstate;
>         struct rtattr *vf[IFLA_VF_MAX + 1] = {};
> -       struct rtattr *tmp;
>
>         SPRINT_BUF(b1);
>
> @@ -323,31 +320,6 @@ static void print_vfinfo(FILE *fp, struct rtattr *vfinfo)
>         vf_vlan = RTA_DATA(vf[IFLA_VF_VLAN]);
>         vf_tx_rate = RTA_DATA(vf[IFLA_VF_TX_RATE]);
>
> -       /* Check if the spoof checking vf info type is supported by
> -        * this kernel.
> -        */
> -       tmp = (struct rtattr *)((char *)vf[IFLA_VF_TX_RATE] +
> -                               vf[IFLA_VF_TX_RATE]->rta_len);
> -
> -       if (tmp->rta_type != IFLA_VF_SPOOFCHK)
> -               vf_spoofchk = NULL;
> -       else
> -               vf_spoofchk = RTA_DATA(vf[IFLA_VF_SPOOFCHK]);
> -
> -       if (vf_spoofchk) {
> -               /* Check if the link state vf info type is supported by
> -                * this kernel.
> -                */
> -               tmp = (struct rtattr *)((char *)vf[IFLA_VF_SPOOFCHK] +
> -                                       vf[IFLA_VF_SPOOFCHK]->rta_len);
> -
> -               if (tmp->rta_type != IFLA_VF_LINK_STATE)
> -                       vf_linkstate = NULL;
> -               else
> -                       vf_linkstate = RTA_DATA(vf[IFLA_VF_LINK_STATE]);
> -       } else
> -               vf_linkstate = NULL;
> -
>         fprintf(fp, "%s    vf %d MAC %s", _SL_, vf_mac->vf,
>                 ll_addr_n2a((unsigned char *)&vf_mac->mac,
>                             ETH_ALEN, 0, b1, sizeof(b1)));
> @@ -366,14 +338,18 @@ static void print_vfinfo(FILE *fp, struct rtattr *vfinfo)
>                 if (vf_rate->min_tx_rate)
>                         fprintf(fp, ", min_tx_rate %dMbps", vf_rate->min_tx_rate);
>         }
> +       if (vf[IFLA_VF_SPOOFCHK]) {
> +               struct ifla_vf_spoofchk *vf_spoofchk =
> +                       RTA_DATA(vf[IFLA_VF_SPOOFCHK]);
>
> -       if (vf_spoofchk && vf_spoofchk->setting != -1) {
> -               if (vf_spoofchk->setting)
> -                       fprintf(fp, ", spoof checking on");
> -               else
> -                       fprintf(fp, ", spoof checking off");
> +               if (vf_spoofchk->setting != -1)
> +                       fprintf(fp, ", spoof checking %s",
> +                               vf_spoofchk->setting ? "on" : "off");
>         }
> -       if (vf_linkstate) {
> +       if (vf[IFLA_VF_LINK_STATE]) {
> +               struct ifla_vf_link_state *vf_linkstate =
> +                       RTA_DATA(vf[IFLA_VF_LINK_STATE]);
> +
>                 if (vf_linkstate->link_state == IFLA_VF_LINK_STATE_AUTO)
>                         fprintf(fp, ", link-state auto");
>                 else if (vf_linkstate->link_state == IFLA_VF_LINK_STATE_ENABLE)
> --
> 2.8.2
>

Re: [iproute PATCH 1/2] ipaddress: Simplify vf_info parsing

[Phil Sutter]

On Wed, Jun 01, 2016 at 03:00:08PM -0700, Greg Rose wrote:
> On Wed, Jun 1, 2016 at 1:03 PM, Phil Sutter <phil@nwl.cc> wrote:
> > Not sure whether I misinterpret commit 7b8179c780a1a, but it looks
> > overly complicated. Instead rely upon parse_rtattr_nested() to assign
> > the relevant pointer if requested rtattr fields are present.
> 
> I'm not sure if newer iproute2 utilities are supposed to work on older
> kernels but if it is you may want to check this against a 2.6.32
> kernel.

Yes, it is supposed to. Actually I tried, but the old RHEL6 kernel I
used didn't export the VF list at all and then I lost motivation.

I didn't check all earlier versions of 7b8179c780a1a, was there a stage
when it looked like what I'm changing it to?

Cheers, Phil

Re: [iproute PATCH 1/2] ipaddress: Simplify vf_info parsing

[Greg Rose]

On Wed, Jun 1, 2016 at 3:07 PM, Phil Sutter <phil@nwl.cc> wrote:
> On Wed, Jun 01, 2016 at 03:00:08PM -0700, Greg Rose wrote:
>> On Wed, Jun 1, 2016 at 1:03 PM, Phil Sutter <phil@nwl.cc> wrote:
>> > Not sure whether I misinterpret commit 7b8179c780a1a, but it looks
>> > overly complicated. Instead rely upon parse_rtattr_nested() to assign
>> > the relevant pointer if requested rtattr fields are present.
>>
>> I'm not sure if newer iproute2 utilities are supposed to work on older
>> kernels but if it is you may want to check this against a 2.6.32
>> kernel.
>
> Yes, it is supposed to. Actually I tried, but the old RHEL6 kernel I
> used didn't export the VF list at all and then I lost motivation.
>
> I didn't check all earlier versions of 7b8179c780a1a, was there a stage
> when it looked like what I'm changing it to?

I don't think so but your patch looks correct - I mean it looks like
it should work.

It's been 5 years since I wrote that original patch and my memory
isn't so great as to why I didn't just do as your patch does but I
think it had something to do with not all drivers reporting a spoof
check value.  However, your patch should handle that case so I see no
reason not to accept it.  Unfortunately I don't have time or the
resources at the moment to check it on an older kernel.

- Greg

>
> Cheers, Phil

Re: [iproute PATCH 1/2] ipaddress: Simplify vf_info parsing

[Phil Sutter]

On Wed, Jun 01, 2016 at 03:36:09PM -0700, Greg Rose wrote:
> On Wed, Jun 1, 2016 at 3:07 PM, Phil Sutter <phil@nwl.cc> wrote:
> > On Wed, Jun 01, 2016 at 03:00:08PM -0700, Greg Rose wrote:
> >> On Wed, Jun 1, 2016 at 1:03 PM, Phil Sutter <phil@nwl.cc> wrote:
> >> > Not sure whether I misinterpret commit 7b8179c780a1a, but it looks
> >> > overly complicated. Instead rely upon parse_rtattr_nested() to assign
> >> > the relevant pointer if requested rtattr fields are present.
> >>
> >> I'm not sure if newer iproute2 utilities are supposed to work on older
> >> kernels but if it is you may want to check this against a 2.6.32
> >> kernel.
> >
> > Yes, it is supposed to. Actually I tried, but the old RHEL6 kernel I
> > used didn't export the VF list at all and then I lost motivation.
> >
> > I didn't check all earlier versions of 7b8179c780a1a, was there a stage
> > when it looked like what I'm changing it to?
> 
> I don't think so but your patch looks correct - I mean it looks like
> it should work.
> 
> It's been 5 years since I wrote that original patch and my memory
> isn't so great as to why I didn't just do as your patch does but I
> think it had something to do with not all drivers reporting a spoof
> check value.  However, your patch should handle that case so I see no
> reason not to accept it.  Unfortunately I don't have time or the
> resources at the moment to check it on an older kernel.

So can I count that as your Acked-by? ;)
Looks like Stephen hesitates to accept this patch due to the discussion
it provoked.

Cheers, Phil

Re: [iproute PATCH 1/2] ipaddress: Simplify vf_info parsing

[Phil Sutter]

On Wed, Jun 01, 2016 at 03:36:09PM -0700, Greg Rose wrote:
> On Wed, Jun 1, 2016 at 3:07 PM, Phil Sutter <phil@nwl.cc> wrote:
> > On Wed, Jun 01, 2016 at 03:00:08PM -0700, Greg Rose wrote:
> >> On Wed, Jun 1, 2016 at 1:03 PM, Phil Sutter <phil@nwl.cc> wrote:
> >> > Not sure whether I misinterpret commit 7b8179c780a1a, but it looks
> >> > overly complicated. Instead rely upon parse_rtattr_nested() to assign
> >> > the relevant pointer if requested rtattr fields are present.
> >>
> >> I'm not sure if newer iproute2 utilities are supposed to work on older
> >> kernels but if it is you may want to check this against a 2.6.32
> >> kernel.
> >
> > Yes, it is supposed to. Actually I tried, but the old RHEL6 kernel I
> > used didn't export the VF list at all and then I lost motivation.
> >
> > I didn't check all earlier versions of 7b8179c780a1a, was there a stage
> > when it looked like what I'm changing it to?
> 
> I don't think so but your patch looks correct - I mean it looks like
> it should work.
> 
> It's been 5 years since I wrote that original patch and my memory
> isn't so great as to why I didn't just do as your patch does but I
> think it had something to do with not all drivers reporting a spoof
> check value.  However, your patch should handle that case so I see no
> reason not to accept it.  Unfortunately I don't have time or the
> resources at the moment to check it on an older kernel.

OK, I checked the code and your v1 patch again. Here it is for
reference:

http://www.spinics.net/lists/netdev/msg175377.html

The issue back then was related to the existence of struct ifla_vf_info
which you extended and thus created and incompatibility with older
kernel's headers. This potential problem went away with the introduction
of uapi headers in kernel commit 607ca46e97a1b, as in
include/uapi/linux/if_link.h named structure doesn't even exist anymore.

The netlink interface between kernel and userspace is safe in that
regard: The returned message is merely a list of struct rtattr buffers,
and parse_rtattr_nested() as used in print_vfinfo() iterates over this
list and populates struct rtattr *vf[] with pointers into that buffer.
So in case a given identifier (IFLA_VF_SPOOFCHK in this case) is not
present in the message returned by the kernel, the respective field in
'vf' will just remain a null pointer.

Please note that the above applies to recent kernels as well as older
ones, so the procedure is backward-compatible.

Furthermore, my patch eliminates a potential bug in iproute2 which is
triggered when the kernel at some point decides to not send the
IFLA_VF_SPOOFCHK buffer immediately after IFLA_VF_TX_RATE. This is
perfectly valid in netlink, thus such assumption categorically wrong.

Stephen, please reconsider this patch as it merely aligns print_vfinfo()
with all other places parsing rtnetlink messages, like e.g.
print_linkinfo().

Thanks, Phil

Re: [iproute PATCH 1/2] ipaddress: Simplify vf_info parsing

[Phil Sutter]

On Wed, Jun 01, 2016 at 10:03:49PM +0200, Phil Sutter wrote:
> Not sure whether I misinterpret commit 7b8179c780a1a, but it looks
> overly complicated. Instead rely upon parse_rtattr_nested() to assign
> the relevant pointer if requested rtattr fields are present.

In order to validate correctness of this patch, I compiled a kernel
which does not export IFLA_VF_SPOOFCHK and verified correct
functionality.

Is there anything else I could do in order to convince you to accept
these patches?

Thanks, Phil