* [iproute PATCH 0/2] Resend: Simplify and enhance vf_info parsing @ 2016-11-08 21:29 Phil Sutter 2016-11-08 21:29 ` [iproute PATCH 1/2] ipaddress: Simplify " Phil Sutter 2016-11-08 21:29 ` [iproute PATCH 2/2] ipaddress: Print IFLA_VF_QUERY_RSS_EN setting Phil Sutter 0 siblings, 2 replies; 4+ messages in thread From: Phil Sutter @ 2016-11-08 21:29 UTC (permalink / raw) To: Stephen Hemminger; +Cc: netdev 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* [iproute PATCH 1/2] ipaddress: Simplify vf_info parsing 2016-11-08 21:29 [iproute PATCH 0/2] Resend: Simplify and enhance vf_info parsing Phil Sutter @ 2016-11-08 21:29 ` Phil Sutter 2016-11-08 23:05 ` Greg 2016-11-08 21:29 ` [iproute PATCH 2/2] ipaddress: Print IFLA_VF_QUERY_RSS_EN setting Phil Sutter 1 sibling, 1 reply; 4+ messages in thread From: Phil Sutter @ 2016-11-08 21:29 UTC (permalink / raw) To: Stephen Hemminger; +Cc: netdev 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [iproute PATCH 1/2] ipaddress: Simplify vf_info parsing 2016-11-08 21:29 ` [iproute PATCH 1/2] ipaddress: Simplify " Phil Sutter @ 2016-11-08 23:05 ` Greg 0 siblings, 0 replies; 4+ messages in thread From: Greg @ 2016-11-08 23:05 UTC (permalink / raw) To: Phil Sutter; +Cc: Stephen Hemminger, netdev 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) ^ permalink raw reply [flat|nested] 4+ messages in thread
* [iproute PATCH 2/2] ipaddress: Print IFLA_VF_QUERY_RSS_EN setting 2016-11-08 21:29 [iproute PATCH 0/2] Resend: Simplify and enhance vf_info parsing Phil Sutter 2016-11-08 21:29 ` [iproute PATCH 1/2] ipaddress: Simplify " Phil Sutter @ 2016-11-08 21:29 ` Phil Sutter 1 sibling, 0 replies; 4+ messages in thread From: Phil Sutter @ 2016-11-08 21:29 UTC (permalink / raw) To: Stephen Hemminger; +Cc: netdev 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-11-08 23:05 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-11-08 21:29 [iproute PATCH 0/2] Resend: Simplify and enhance vf_info parsing Phil Sutter 2016-11-08 21:29 ` [iproute PATCH 1/2] ipaddress: Simplify " Phil Sutter 2016-11-08 23:05 ` Greg 2016-11-08 21:29 ` [iproute PATCH 2/2] ipaddress: Print IFLA_VF_QUERY_RSS_EN setting Phil Sutter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).