netdev - Re: [PATCH iproute2] ip: fix link type and vlan oneline output

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <1671392030.4223005.1579625954850.JavaMail.zimbra@redhat.com>
Date:   Tue, 21 Jan 2020 11:59:14 -0500 (EST)
From:   Vladis Dronov <vdronov@...hat.com>
To:     Stephen Hemminger <stephen@...workplumber.org>
Cc:     netdev@...r.kernel.org, David Ahern <dsahern@...il.com>,
        George Shuklin <george.shuklin@...il.com>
Subject: Re: [PATCH iproute2] ip: fix link type and vlan oneline output

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@...h90: <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@...h90: ... 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@...h90: ... 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@...workplumber.org>
> To: "Vladis Dronov" <vdronov@...hat.com>
> Cc: netdev@...r.kernel.org, "David Ahern" <dsahern@...il.com>, "George Shuklin" <george.shuklin@...il.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.