Fix broken code and APIs in Linux networking paths #1656
Conversation
There was a problem hiding this comment.
Looks ok, just want to check on returning renderer values in code callable anywhere in the framework (convention is to leave it as None until the moment it's going to be rendered). And also using a facade to keep an old function around for little reason... The old code's never gonna get called, so it's just adding an extra hidden function with no value...
|
|
||
| # Interface IPv6 Addresses | ||
| inet6_dev = net_dev.ip6_ptr.dereference().cast("inet6_dev") | ||
| for inet6_ifaddr in inet6_dev.get_addresses(): | ||
| prefix_len = inet6_ifaddr.get_prefix_len() | ||
| scope_type = inet6_ifaddr.get_scope_type() | ||
| ip6_addr = inet6_ifaddr.get_address() | ||
| yield net_ns_id, iface_ifindex, iface_name, mac_addr, promisc, ip6_addr, prefix_len, scope_type, operational_state | ||
| yield net_ns_id or renderers.NotAvailableValue(), iface_ifindex, iface_name, mac_addr, promisc, ip6_addr, prefix_len, scope_type, operational_state |
There was a problem hiding this comment.
I thought we were favouring returning None and then at render time doing or NotAvailableValue()?
There was a problem hiding this comment.
I left this as acceptable here b/c the function is just gathering the data directly sent through the generator. If we make it return None in the line you highlighted then this line in the generator (this is a link to the full file) would need to unpack all those fields (10 or so) versus just passing it through:
There was a problem hiding this comment.
Mmmmm, not sure how I feel about that. It's a good reason but given this is going to be reused a lot, I'd prefer we did the unpacking (once) rather than letting the RendererValues sneak further into the API. It's much harder to convince people not to do this stuff, if they can point to a core example doing it... 5:S It's a little more work now, but it'll keep everything future and dependable in the future. Could you make the changes please?
atcuno
force-pushed
the
fix_linux_networking_paths
branch
from
f947653 to
6a9a7bc
Compare
|
@ikelos for this one I left you replies to both your comments. |
ikelos
added
the
awaiting-author-response
The Linux networking paths that supported the two
ipplugins had many issues, including aget_*throwing a paging exception, not catching exceptions for reading names, and other issues. These caused backtraces all over in testing. These are now fixed.