Created attachment 152475 [details] patch The following patch reorganizes the list of addresses associated with an interface and group them based on the address family. This should help to recognize interfaces with multiple AF (e.g. ipv4 and ipv6) with many aliases or additional addresses. The order of addresses inside a single group is strictly preserved. Moreover, this patch improves scope_id output for AF_INET6 families, as the current approach uses hexadecimal string that is basically the ID of an interface, whilst this information is already depicted by getnameinfo(3) call. Therefore, now ifconfig just prints the scope of address as it is defined in 2.4 of RFC 2373. Example of output: Original ifconfig: # ifconfig vlan1 vlan1: flags=8943<UP,BROADCAST,RUNNING,PROMISC,SIMPLEX,MULTICAST> metric 0 mtu 1500 options=3<RXCSUM,TXCSUM> ether 00:1a:64:c6:a8:7c inet 192.168.3.22 netmask 0xffffff00 broadcast 192.168.3.255 inet6 fe80::21a:64ff:fec6:a87c%vlan1 prefixlen 64 scopeid 0x6 inet6 fd00::316 prefixlen 120 inet 192.168.3.13 netmask 0xffffff00 broadcast 192.168.3.255 vhid 5 inet 192.168.3.1 netmask 0xffffff00 broadcast 192.168.3.255 vhid 5 inet6 fd00::301 prefixlen 120 vhid 5 nd6 options=21<PERFORMNUD,AUTO_LINKLOCAL> media: Ethernet autoselect (1000baseT <full-duplex>) status: active vlan: 1 parent interface: bce0 carp: MASTER vhid 5 advbase 1 advskew 50 Modified output: # ./ifconfig vlan1 vlan1: flags=8943<UP,BROADCAST,RUNNING,PROMISC,SIMPLEX,MULTICAST> metric 0 mtu 1500 options=3<RXCSUM,TXCSUM> inet6 fe80::21a:64ff:fec6:a87c%vlan1 prefixlen 64 scope: Link inet6 fd00::316 prefixlen 120 scope: Global inet6 fd00::301 prefixlen 120 scope: Global vhid 5 inet 192.168.3.22 netmask 0xffffff00 broadcast 192.168.3.255 inet 192.168.3.13 netmask 0xffffff00 broadcast 192.168.3.255 vhid 5 inet 192.168.3.1 netmask 0xffffff00 broadcast 192.168.3.255 vhid 5 ether 00:1a:64:c6:a8:7c nd6 options=21<PERFORMNUD,AUTO_LINKLOCAL> media: Ethernet autoselect (1000baseT <full-duplex>) status: active vlan: 1 parent interface: bce0 carp: MASTER vhid 5 advbase 1 advskew 50
Created attachment 152478 [details] patch
Created attachment 152479 [details] patch
That looks good to me. Please commit
A commit references this bug: Author: vsevolod Date: Mon Feb 2 13:03:05 UTC 2015 New revision: 278080 URL: https://svnweb.freebsd.org/changeset/base/278080 Log: Reorganize the list of addresses associated with an interface and group them based on the address family. This should help to recognize interfaces with multiple AF (e.g. ipv4 and ipv6) with many aliases or additional addresses. The order of addresses inside a single group is strictly preserved. Improve the scope_id output for AF_INET6 families, as the current approach uses hexadecimal string that is basically the ID of an interface, whilst this information is already depicted by getnameinfo(3) call. Therefore, now ifconfig just prints the scope of address as it is defined in 2.4 of RFC 2373. PR: 197270 Approved by: bapt MFC after: 2 weeks Changes: head/sbin/ifconfig/af_inet6.c head/sbin/ifconfig/ifconfig.c
It's rather unfortunate that this did not g through a proper review process and was handled within a few minutes of reporting by yourself. The scope change doesn't look particularly great to me at all. Also the reference in the commit message (RFC 2373) is not good as that RFC has been obsoleted at least twice and updated a couple of times. Also changing the output format of ifconfig unfortunately break a lot of scripts of people usually so should have been handled carefully. I strongly object to an MFC of the scope part at least. I am also worried that not having the "inet" first anymore will break scripts (the much I prefer IPv6 being a default). Another reason to be very careful with MFCing this.
I have added ae@, who suggested me that change.
Regarding order, I believe that the change to revert it is completely trivial - just change the order of AF in `cmpifaddrs' function.
(In reply to commit-hook from comment #4) > Reorganize the list of addresses associated with an interface and group them > based on the address family. This should help to recognize interfaces with > multiple AF (e.g. ipv4 and ipv6) with many aliases or additional addresses. The > order of addresses inside a single group is strictly preserved. > > Improve the scope_id output for AF_INET6 families, as the > current approach uses hexadecimal string that is basically the ID of an > interface, whilst this information is already depicted by getnameinfo(3) call. > Therefore, now ifconfig just prints the scope of address as it is defined in > 2.4 of RFC 2373. Please revert the second half of this change. Checking the address structure in userland in the manner of in6_print_scope() is redundant because almost the same check is already performed in the kernel when getifaddrs(3) is called. If scope_id > 0, or IN6_IS_ADDR_LOOPBACK() is true, it is link-local. Site-local is deprecated in RFC 3879 and new implementation MUST NOT support it. Plus, using hard-coded values of address prefixes is inappropriate. This kind of scope check should be performed by only IN6_IS_ADDR_{LINKLOCAL,SITELOCAL,...} macros if you want a strict one. And I am not sure why "Multicast" is added here. The multicast group membership list is returned by getifmaddrs(3), not getifaddrs(3). Also, RFC 4007 specifies that at least non-negative decimal integer SHOULD be supported as scope zone id. We support it. However, if removing the scopeid part, it becomes difficult to know the zone id. This is the reason why scopeid is displayed along with %zone_id. A symbolic "%zone_id" itself means "it is non-global". Is "scope: link" still needed? Putting the above aside, I do not think it is a good idea to change the command line output because it breaks consistency (e.g. ifmcstat(8) also uses scopeid notation) and compatibility of scripts which depend on it, for example. scopeid has lived for 10+ years. While adding "scope: link" or something may be acceptable, replacing scopeid with it does not look a good improvement, at least to me.
Created attachment 152488 [details] reverte parts of patch This patch reverts the scopeid change and set the order of AF in the way it is returned by getifaddrs (previously, this order was reverted).
Hiroki: is the partial revert patch ok for you?
Is it possible to use a qsort(3) function to sort the remaining parts? This discussion should really be in reviews and not here but hey ;-)
(In reply to Baptiste Daroussin from comment #10) Yes, good to me.
Bjoern, using of qsort is impossible for two main reasons: 1) it cannot sort linked lists 2) it is not stable This sorting is a mergesort actually, so it is stable which is important while showing aliases in the correct order.
However, if you've meant sorting of interfaces, then yes, I planned to add support of custom sorting orders for the convenient display of different interfaces.
(In reply to Hiroki Sato from comment #8) > Also, RFC 4007 specifies that at least non-negative decimal integer > SHOULD be supported as scope zone id. We support it. However, if > removing the scopeid part, it becomes difficult to know the zone id. Zone id is printed in the same line before "prefixlen" keyword. You can just copy an address with %zone_id and use it where you want. And I'm sure you are doing so. > This is the reason why scopeid is displayed along with %zone_id. > A symbolic "%zone_id" itself means "it is non-global". > Is "scope: link" still needed? So, why this zone index is formatted in hex? We don't support hexadecimal values. It looks like many people use them every day... :) > Putting the above aside, I do not think it is a good idea to change > the command line output because it breaks consistency > (e.g. ifmcstat(8) also uses scopeid notation) and compatibility of ifmcstat(8) also has this redundant information. > scripts which depend on it, for example. scopeid has lived for 10+ > years. While adding "scope: link" or something may be acceptable, > replacing scopeid with it does not look a good improvement, at least > to me.
To originators/assignees of this PR: A commit to the tree references this PR, however the PR is still in a non-closed state. Please review this PR and close as appropriate, or if closing the PR requires a merge to stable/10, please let re@ know as soon as possible. Thank you. Glen