GNOME Bugzilla – Bug 697525
ifcfg-rh plugin doesn't properly handle legacy route files without gateway
Last modified: 2017-09-27 07:51:55 UTC
Created attachment 240915 [details] [review] Patch to reader.c to handle missing gateway in route files The route configuration for an interface can include entries without a route, eg /etc/sysconfig/network-scripts/route-br0: # add multicast routing for interface 224.0.0.0/4 dev br0 Such routes are legal "link scoped" routes, and the initscripts documentation (sysconfig.txt) actually includes an example for route-<interface-name> of 192.168.2.0/24 dev ppp0 However, creating such a route file causes the ifcfg-rh plugin to fail with the error: NetworkManager[646]: ifcfg-rh: error: Missing IP4 route gateway address in record: '224.0.0.0/4 dev br0' and brings down the interface (which is actually quite painful when you happen to be editing the file over the network :) I checked the source code in reader.c, and found that the problem can be worked around with an entry such as: 224.0.0.0/4 via 0.0.0.0 dev br0 but really should be handled gracefully (esp with relation to the documented example). I've created a patch to reader.c that treats a missing gateway the same way as when it's missing from the "new format" route files, ie as "via 0.0.0.0"
NM bugzilla reorganization... sorry for the bug spam.
Hi, This looks good to me. I applied the patch to current master and after review it might get merged. http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?h=thaller/bug-gn-697525-ifcfg-rh-parse-missing-route&id=8c4732447940fa98406db5d8c02deb9a5b839740 @Scott, I assume you agree being named as author of the patch (and Reported-by:)? Please see the link above. If not, I will of course change it. Thank you for reporting!! Thomas
The only thing I'd suggest is that the updated code would make more sense if you removed the "!" from the g_match_info_matches(), and put the success case first and the error case as the else.
The amended the commit and put the success case first. Thank you.
Commit pushed to master: http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=04f6e09d5082acaceef2329675d8470cf23f202f One remaining question is about IPv6 route files. sysconfig.txt says: /etc/sysconfig/network-scripts/route6-<interface-name> Contains lines that are arguments to "/sbin/ip -6 route add" For example: site-local route for network fec0:0:0:2::/64 via gateway fec0:0:0:1:0:0:0:20 (e.g. on eth0):· fec0:0:0:2::/64 via fec0:0:0:1:0:0:0:20 additional prefix configured to be on-link on eth0: 3ffe:fffe:1:2::/64 dev eth0 6to4 route for network 3ffe:ffff:1::/48, either: 3ffe:ffff:1::/48 3ffe:ffff:1::/48 via ::192.168.1.2 Note the special case of 6to4 interface: 'via [relay]' is· automatically added if explicit 'via' wasn't specified. So probably, this should be added for route6-* files as well. Any comments?
(In reply to comment #0) > and brings down the interface (which is actually quite painful when you happen > to be editing the file over the network :) Note that NetworkManager is moving towards explicit reload only. The current master already supports setting 'monitor-connection-files' off, see NetworkManager.conf(5). (In reply to comment #5) > So probably, this should be added for route6-* files as well. Definitely.
Comment on attachment 240915 [details] [review] Patch to reader.c to handle missing gateway in route files a slightly different patch was committed
... if this is committed to master, should we close it?
a patch was committed, but comment 5 says "So probably, this should be added for route6-* files as well."
(In reply to comment #9) > a patch was committed, but comment 5 says "So probably, this should be added > for route6-* files as well." The route6- file change is at jk/ifcfg-rh-route6-bgo697525
Created attachment 278204 [details] [review] [PATCH] fix several issues parsing IPv6 routes in ifcfg-rh This whole parsing of initscripts is such a patchwork... I think, ip6 routes are only valid without 'via' if - they are on-link (i.e. when specifying 'dev' too) - they are 6to4 but since we don't do any careful parsing, we don't know which case we have at hand and might accept some routes that are actually invalid. Also, we accept "default/64", which is not valid in initscripts. Also, we skip over "default", but we allow "::" -- which effectively has the same meaning. Also, we ignore any unrecognised values. Probably because initscripts (which passes the line on to `ip -6 route add` would support it). Also, I think that we should clear out the host part of a destination. E.g. ab::1/64 should be cleared to ab::/64. Initscripts/`ip route add` seems to just behave like that. Also, with initscripts you could specify for example: "abbe::cafe/64 metric 777 proto static" which NM silently would accept as "abbe::cafe/64 metric 777" This seems wrong, because if NM cannot support a certain specifier, we should reject it or at least warn about it. Also, with initscripts you could not specify "abbe::cafe/64 via x metric 777" but NM would accept it equal to: "abbe::cafe/64 via :: metric 777" Also, with initscripts you could not specify "abbe::cafe/64 metric777" but NM would accept it equal to: "abbe::cafe/64 While playing around, I tried to fix the above issues, and came up with the attached patch. But maybe this is all just bottomless... (what do you think?) So, I would be almost OK with ignoring all these issues and merge jk/ifcfg-rh-route6-bgo697525 as is.
The main issue is that the route6 file format is just a string passed to "ip route add". The format can be quite complex and is not very easy to parse. ifup-routes script just passes the string to "ip route add". For IPv4, the initscripts (ifup-routes) also accepts newer format with variables that it easy to parse and check. Unfortunately it is not used for IPv6, probably because they use ipcalc program and that doesn't support IPv6. But I think it would be quite hard to duplicate route parsing of "iproute2" in NetworkManager. It would be much easier to update initscripts to accept better format (ADDRESS0, PREFIX0,NEXT_HOP0, METRIC0, ...). So I commit the change of jk/ifcfg-rh-route6-bgo697525 now: ee3ac2 ifcfg-rh: accept IPv6 routes without "via" in route6 file (bgo #697525) And we can discuss any enhancements later; leaving the bug open for now.
(removing nm-review since patch was reviewed and pushed)
Let me know if you want to do anything with either initscripts or iproute2.
in the meantime the parsing code of route files was completely refactored. It no longer uses regex to parse the lines, instead it tokenizes the line (like initscripts do) and strictly parses all fields that it understands -- rejecting words that it doesn't understand. The parser is supposed to behave identical to what iproute2 does. If not, please open a new bug. See https://cgit.freedesktop.org/NetworkManager/NetworkManager/tree/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c?id=e6292bcb2a1326dd2ff4f53933acb76b8724e6d9#n520 This bug is hence fixed.