| Message ID | 20200119011251.7153-1-vdronov@redhat.com |
|---|---|
| State | Accepted |
| Delegated to: | stephen hemminger |
| Headers | show |
| Series | [iproute2] ip: fix link type and vlan oneline output | expand |
hello, i believe this fix should get to iproute2-next too and i'm not sure how to do it. could you, please, advise/help to handle this? -vladis
On Sun, 19 Jan 2020 02:12:51 +0100 Vladis Dronov <vdronov@redhat.com> wrote: > Move link type printing in print_linkinfo() so multiline output does not > break link options line. Add oneline support for vlan's ingress and egress > qos maps. > > Before the fix: > > # ip -details link show veth90.4000 > 5: veth90.4000@veth90: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000 > link/ether 26:9a:05:af:db:00 brd ff:ff:ff:ff:ff:ff promiscuity 0 minmtu 0 maxmtu 65535 > vlan protocol 802.1Q id 4000 <REORDER_HDR> the option line is broken ^^^ > ingress-qos-map { 1:2 } > egress-qos-map { 2:1 } addrgenmode eui64 numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535 > > # ip -oneline -details link show veth90.4000 > 5: veth90.4000@veth90: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000\ link/ether 26:9a:05:af:db:00 brd ff:ff:ff:ff:ff:ff promiscuity 0 minmtu 0 maxmtu 65535 \ vlan protocol 802.1Q id 4000 <REORDER_HDR> > ingress-qos-map { 1:2 } <<< a multiline output despite -oneline > egress-qos-map { 2:1 } addrgenmode eui64 numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535 > > After the fix: > > # ip -details link show veth90.4000 > 5: veth90.4000@veth90: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000 > link/ether 26:9a:05:af:db:00 brd ff:ff:ff:ff:ff:ff promiscuity 0 minmtu 0 maxmtu 65535 addrgenmode eui64 numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535 > vlan protocol 802.1Q id 4000 <REORDER_HDR> > ingress-qos-map { 1:2 } > egress-qos-map { 2:1 } > > # ip -oneline -details link show veth90.4000 > 5: veth90.4000@veth90: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000\ link/ether 26:9a:05:af:db:00 brd ff:ff:ff:ff:ff:ff promiscuity 0 minmtu 0 maxmtu 65535 addrgenmode eui64 numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535 \ vlan protocol 802.1Q id 4000 <REORDER_HDR> \ ingress-qos-map { 1:2 } \ egress-qos-map { 2:1 } > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=206241 > Reported-by: George Shuklin <george.shuklin@gmail.com> > Signed-off-by: Vladis Dronov <vdronov@redhat.com> Applied with changes. The change to ipaddress.c was incorrect. You can't change the order of things in the output. Second, you needed to have a Fixes tag. In this case, it went back to original vlan support.
hello, > The change to ipaddress.c was incorrect. You can't change the order of things > in the output. i still believe it is bad to break a single link layers options line with a multi-line piece: 5: veth90.4000@veth90: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000 link/ether 26:9a:05:af:db:00 brd ff:ff:ff:ff:ff:ff promiscuity 0 minmtu 0 maxmtu 65535 vlan protocol 802.1Q id 4000 <REORDER_HDR> the option line is broken ^^^ by \n in print_linktype() ingress-qos-map { 1:2 } egress-qos-map { 2:1 } addrgenmode eui64 numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535 ^^^ the option line continues here print_linktype() has an unconditional \n so it always breaks a link layer options line. this output is assumed to be human-readable, so i believe, proper indentation is more important that an order of fields. a machine- readable format is another one, as far as i understand. i'm not sure how "an order of things" is supposed to be stable, if print_linktype() outputs a variable number of variable fields (depending on a link type) in the middle of a link layer options. surely, if an order of things in this understanding is above all, please, ignore my rant. also, with the change a commit message should be different. i understand, it is too late to change it, but still. let a proper message be at least here, in the mail thread: >>> begin >>> Add oneline support for vlan's ingress and egress qos maps. Before the fix: # ip -oneline -details link show veth90.4000 5: veth90.4000@veth90: ... maxmtu 65535 \ vlan protocol 802.1Q id 4000 <REORDER_HDR> ingress-qos-map { 1:2 } <<< a multiline output despite -oneline egress-qos-map { 2:1 } addrgenmode eui64 numtxqueues 1 ... After the fix: # ip -details link show veth90.4000 5: veth90.4000@veth90: ... maxmtu 65535 \ vlan protocol 802.1Q id 4000 <REORDER_HDR> \ ingress-qos-map { 1:2 } \ egress-qos-map { 2:1 } addrgenmode eui64 numtxqueues 1 ... <<< end <<< > Second, you needed to have a Fixes tag. In this case, it went back to > original vlan support. indeed, this is my mistake, thank you for correcting this in the patch submitted. Best regards, Vladis Dronov | Red Hat, Inc. | The Core Kernel | Senior Software Engineer ----- Original Message ----- > From: "Stephen Hemminger" <stephen@networkplumber.org> > To: "Vladis Dronov" <vdronov@redhat.com> > Cc: netdev@vger.kernel.org, "David Ahern" <dsahern@gmail.com>, "George Shuklin" <george.shuklin@gmail.com> > Sent: Monday, January 20, 2020 6:34:04 PM > Subject: Re: [PATCH iproute2] ip: fix link type and vlan oneline output > > The change to ipaddress.c was incorrect. You can't change the order of things > in the output. > > Second, you needed to have a Fixes tag. In this case, it went back to > original vlan support.
diff --git a/ip/ipaddress.c b/ip/ipaddress.c index 964f14df..8814c298 100644 --- a/ip/ipaddress.c +++ b/ip/ipaddress.c @@ -1072,9 +1072,6 @@ int print_linkinfo(struct nlmsghdr *n, void *arg) "max_mtu", "maxmtu %u ", rta_getattr_u32(tb[IFLA_MAX_MTU])); - if (tb[IFLA_LINKINFO]) - print_linktype(fp, tb[IFLA_LINKINFO]); - if (do_link && tb[IFLA_AF_SPEC]) print_af_spec(fp, tb[IFLA_AF_SPEC]); @@ -1126,6 +1123,9 @@ int print_linkinfo(struct nlmsghdr *n, void *arg) RTA_PAYLOAD(tb[IFLA_PHYS_SWITCH_ID]), b1, sizeof(b1))); } + + if (tb[IFLA_LINKINFO]) + print_linktype(fp, tb[IFLA_LINKINFO]); } if ((do_link || show_details) && tb[IFLA_IFALIAS]) { diff --git a/ip/iplink_vlan.c b/ip/iplink_vlan.c index 0dfb4a8d..1e6817f5 100644 --- a/ip/iplink_vlan.c +++ b/ip/iplink_vlan.c @@ -183,7 +183,8 @@ static void vlan_print_map(FILE *f, int rem; open_json_array(PRINT_JSON, name_json); - print_string(PRINT_FP, NULL, "\n %s { ", name_fp); + print_nl(); + print_string(PRINT_FP, NULL, " %s { ", name_fp); rem = RTA_PAYLOAD(attr); for (i = RTA_DATA(attr); RTA_OK(i, rem); i = RTA_NEXT(i, rem)) {
Move link type printing in print_linkinfo() so multiline output does not break link options line. Add oneline support for vlan's ingress and egress qos maps. Before the fix: # ip -details link show veth90.4000 5: veth90.4000@veth90: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000 link/ether 26:9a:05:af:db:00 brd ff:ff:ff:ff:ff:ff promiscuity 0 minmtu 0 maxmtu 65535 vlan protocol 802.1Q id 4000 <REORDER_HDR> the option line is broken ^^^ ingress-qos-map { 1:2 } egress-qos-map { 2:1 } addrgenmode eui64 numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535 # ip -oneline -details link show veth90.4000 5: veth90.4000@veth90: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000\ link/ether 26:9a:05:af:db:00 brd ff:ff:ff:ff:ff:ff promiscuity 0 minmtu 0 maxmtu 65535 \ vlan protocol 802.1Q id 4000 <REORDER_HDR> ingress-qos-map { 1:2 } <<< a multiline output despite -oneline egress-qos-map { 2:1 } addrgenmode eui64 numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535 After the fix: # ip -details link show veth90.4000 5: veth90.4000@veth90: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000 link/ether 26:9a:05:af:db:00 brd ff:ff:ff:ff:ff:ff promiscuity 0 minmtu 0 maxmtu 65535 addrgenmode eui64 numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535 vlan protocol 802.1Q id 4000 <REORDER_HDR> ingress-qos-map { 1:2 } egress-qos-map { 2:1 } # ip -oneline -details link show veth90.4000 5: veth90.4000@veth90: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000\ link/ether 26:9a:05:af:db:00 brd ff:ff:ff:ff:ff:ff promiscuity 0 minmtu 0 maxmtu 65535 addrgenmode eui64 numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535 \ vlan protocol 802.1Q id 4000 <REORDER_HDR> \ ingress-qos-map { 1:2 } \ egress-qos-map { 2:1 } Link: https://bugzilla.kernel.org/show_bug.cgi?id=206241 Reported-by: George Shuklin <george.shuklin@gmail.com> Signed-off-by: Vladis Dronov <vdronov@redhat.com> --- ip/ipaddress.c | 6 +++--- ip/iplink_vlan.c | 3 ++- 2 files changed, 5 insertions(+), 4 deletions(-)