From 1111472af7b48d9007529a3b5a8d2339ddace8e6 Mon Sep 17 00:00:00 2001 From: Martin Stein Date: Thu, 5 Aug 2021 10:41:47 +0200 Subject: [PATCH] nic_router: make Ipv4_config a class The fact that the IPv4 config was a struct with all data members public was a mere leftover of an early state of the NIC router. Today, the router implementation style is to avoid structs and public data members wherever possible. This commit slightly changes the behavior of the router regarding log output. The router used to print malformed IPv4 configurations to the log only if the 'verbose' config flag was set using this style: ! [my_domain] malformed dynamic IP config: interface 10.0.2.1/24 ... Now, malformed IPv4 configurations are only printed if the 'verbose_domain_state' config flag is set (like with any IP4v configuration states) using this style: ! [my_domain] dynamic IP config: malformed (interface 10.0.2.1/24 ...) Fixes #4242 --- repos/os/src/server/nic_router/dhcp_client.cc | 2 +- repos/os/src/server/nic_router/dhcp_server.cc | 2 +- repos/os/src/server/nic_router/domain.cc | 31 ++----- repos/os/src/server/nic_router/domain.h | 2 +- repos/os/src/server/nic_router/interface.cc | 32 +++---- repos/os/src/server/nic_router/ipv4_config.cc | 50 ++++++---- repos/os/src/server/nic_router/ipv4_config.h | 92 +++++++++++-------- 7 files changed, 112 insertions(+), 99 deletions(-) diff --git a/repos/os/src/server/nic_router/dhcp_client.cc b/repos/os/src/server/nic_router/dhcp_client.cc index a3bc0735cb..4880e12a49 100644 --- a/repos/os/src/server/nic_router/dhcp_client.cc +++ b/repos/os/src/server/nic_router/dhcp_client.cc @@ -72,7 +72,7 @@ void Dhcp_client::discover() void Dhcp_client::_rerequest(State next_state) { _set_state(next_state, _rerequest_timeout(2)); - Ipv4_address const client_ip = _domain().ip_config().interface.address; + Ipv4_address const client_ip = _domain().ip_config().interface().address; _send(Message_type::REQUEST, client_ip, Ipv4_address(), client_ip); } diff --git a/repos/os/src/server/nic_router/dhcp_server.cc b/repos/os/src/server/nic_router/dhcp_server.cc index 29170f0a02..6bed01c856 100644 --- a/repos/os/src/server/nic_router/dhcp_server.cc +++ b/repos/os/src/server/nic_router/dhcp_server.cc @@ -196,7 +196,7 @@ bool Dhcp_server::ready() const if (!_dns_servers.empty()) { return true; } - try { return _dns_server_from().ip_config().valid; } + try { return _dns_server_from().ip_config().valid(); } catch (Pointer::Invalid) { } return true; } diff --git a/repos/os/src/server/nic_router/domain.cc b/repos/os/src/server/nic_router/domain.cc index bac1c58465..0111aaab94 100644 --- a/repos/os/src/server/nic_router/domain.cc +++ b/repos/os/src/server/nic_router/domain.cc @@ -42,20 +42,9 @@ Domain_base::Domain_base(Xml_node const node) void Domain::_log_ip_config() const { - Ipv4_config const &ip_config = *_ip_config; - if (_config.verbose()) { - if (!ip_config.valid && - (ip_config.interface_valid || - ip_config.gateway_valid || - !ip_config.dns_servers.empty())) - { - log("[", *this, "] malformed ", _ip_config_dynamic ? "dynamic" : - "static", "IP config: ", ip_config); - } - } if (_config.verbose_domain_state()) { log("[", *this, "] ", _ip_config_dynamic ? "dynamic" : "static", - " IP config: ", ip_config); + " IP config: ", *_ip_config); } } @@ -66,7 +55,7 @@ void Domain::_prepare_reconstructing_ip_config() throw Ip_config_static(); } /* discard old IP config if any */ - if (ip_config().valid) { + if (ip_config().valid()) { /* mark IP config invalid */ _ip_config.construct(_alloc); @@ -94,7 +83,7 @@ void Domain::_finish_reconstructing_ip_config() _log_ip_config(); /* attach all dependent interfaces to new IP config if it is valid */ - if (ip_config().valid) { + if (ip_config().valid()) { _interfaces.for_each([&] (Interface &interface) { interface.attach_to_ip_config(*this, ip_config()); }); @@ -130,9 +119,9 @@ void Domain::ip_config_from_dhcp_ack(Dhcp_packet &dhcp_ack) void Domain::try_reuse_ip_config(Domain const &domain) { - if (ip_config().valid || + if (ip_config().valid() || !_ip_config_dynamic || - !domain.ip_config().valid || + !domain.ip_config().valid() || !domain._ip_config_dynamic) { return; @@ -261,7 +250,7 @@ void Domain::init(Domain_tree &domains) try { Dhcp_server &dhcp_server = *new (_alloc) Dhcp_server(dhcp_server_node, *this, _alloc, - ip_config().interface, domains); + ip_config().interface(), domains); try { dhcp_server. @@ -355,8 +344,8 @@ void Domain::deinit() Ipv4_address const &Domain::next_hop(Ipv4_address const &ip) const { - if (ip_config().interface.prefix_matches(ip)) { return ip; } - if (ip_config().gateway_valid) { return ip_config().gateway; } + if (ip_config().interface().prefix_matches(ip)) { return ip; } + if (ip_config().gateway_valid()) { return ip_config().gateway(); } throw No_next_hop(); } @@ -401,8 +390,8 @@ void Domain::report(Xml_generator &xml) empty = false; } if (_config.report().config()) { - xml.attribute("ipv4", String<19>(ip_config().interface)); - xml.attribute("gw", String<16>(ip_config().gateway)); + xml.attribute("ipv4", String<19>(ip_config().interface())); + xml.attribute("gw", String<16>(ip_config().gateway())); ip_config().for_each_dns_server([&] (Dns_server const &dns_server) { xml.node("dns", [&] () { xml.attribute("ip", String<16>(dns_server.ip())); diff --git a/repos/os/src/server/nic_router/domain.h b/repos/os/src/server/nic_router/domain.h index 1adfec6288..f92e6d7d9d 100644 --- a/repos/os/src/server/nic_router/domain.h +++ b/repos/os/src/server/nic_router/domain.h @@ -107,7 +107,7 @@ class Net::Domain : public Domain_base, unsigned long _interface_cnt { 0 }; Pointer _dhcp_server { }; Genode::Reconstructible _ip_config; - bool const _ip_config_dynamic { !ip_config().valid }; + bool const _ip_config_dynamic { !ip_config().valid() }; List _ip_config_dependents { }; Arp_cache _arp_cache { *this }; Arp_waiter_list _foreign_arp_waiters { }; diff --git a/repos/os/src/server/nic_router/interface.cc b/repos/os/src/server/nic_router/interface.cc index f8925bdd39..501dcba0a9 100644 --- a/repos/os/src/server/nic_router/interface.cc +++ b/repos/os/src/server/nic_router/interface.cc @@ -414,7 +414,7 @@ void Interface::attach_to_domain_finish() /* if domain has yet no IP config, participate in requesting one */ Domain &domain = _domain(); Ipv4_config const &ip_config = domain.ip_config(); - if (!ip_config.valid) { + if (!ip_config.valid()) { _dhcp_client->discover(); return; } @@ -427,7 +427,7 @@ void Interface::attach_to_ip_config(Domain &domain, { /* if others wait for ARP at the domain, participate in requesting it */ domain.foreign_arp_waiters().for_each([&] (Arp_waiter_list_element &le) { - _broadcast_arp_request(ip_config.interface.address, + _broadcast_arp_request(ip_config.interface().address, le.object()->ip()); }); } @@ -555,7 +555,7 @@ void Interface::_adapt_eth(Ethernet_frame ð, Domain &remote_domain) { Ipv4_config const &remote_ip_cfg = remote_domain.ip_config(); - if (!remote_ip_cfg.valid) { + if (!remote_ip_cfg.valid()) { throw Drop_packet("target domain has yet no IP config"); } if (remote_domain.use_arp()) { @@ -564,7 +564,7 @@ void Interface::_adapt_eth(Ethernet_frame ð, try { eth.dst(remote_domain.arp_cache().find_by_ip(hop_ip).mac()); } catch (Arp_cache::No_match) { remote_domain.interfaces().for_each([&] (Interface &interface) { - interface._broadcast_arp_request(remote_ip_cfg.interface.address, + interface._broadcast_arp_request(remote_ip_cfg.interface().address, hop_ip); }); try { new (_alloc) Arp_waiter { *this, remote_domain, hop_ip, pkt }; } @@ -595,7 +595,7 @@ void Interface::_nat_link_and_pass(Ethernet_frame ð, log("[", local_domain, "] using NAT rule: ", nat); } _src_port(prot, prot_base, nat.port_alloc(prot).alloc()); - ip.src(remote_domain.ip_config().interface.address); + ip.src(remote_domain.ip_config().interface().address); remote_port_alloc = nat.port_alloc(prot); } catch (Nat_rule_tree::No_match) { } @@ -718,7 +718,7 @@ void Interface::_new_dhcp_allocation(Ethernet_frame ð, allocation.ip(), Dhcp_packet::Message_type::OFFER, dhcp.xid(), - local_domain.ip_config().interface); + local_domain.ip_config().interface()); } catch (Out_of_ram) { throw Free_resources_and_retry_handle_eth(); } catch (Out_of_caps) { throw Free_resources_and_retry_handle_eth(); } @@ -743,7 +743,7 @@ void Interface::_handle_dhcp_request(Ethernet_frame ð, _dhcp_allocations.find_by_mac(dhcp.client_mac()); Ipv4_address_prefix const &local_intf = - local_domain.ip_config().interface; + local_domain.ip_config().interface(); switch (msg_type) { case Dhcp_packet::Message_type::DISCOVER: @@ -915,7 +915,7 @@ void Interface::handle_interface_link_state() /* if the whole domain is down, discard IP config */ Domain &domain_ = domain(); - if (!link_state() && domain_.ip_config().valid) { + if (!link_state() && domain_.ip_config().valid()) { domain_.interfaces().for_each([&] (Interface &interface) { if (interface.link_state()) { throw Keep_ip_config(); } @@ -1051,7 +1051,7 @@ void Interface::_handle_icmp_error(Ethernet_frame ð, } /* adapt source and destination of Ethernet frame and IP packet */ _adapt_eth(eth, remote_side.src_ip(), pkt, remote_domain); - if (remote_side.dst_ip() == remote_domain.ip_config().interface.address) { + if (remote_side.dst_ip() == remote_domain.ip_config().interface().address) { ip.src(remote_side.dst_ip()); } ip.dst(remote_side.src_ip()); @@ -1123,7 +1123,7 @@ void Interface::_handle_ip(Ethernet_frame ð, { /* drop fragmented IPv4 as it isn't supported */ Ipv4_packet &ip = eth.data(size_guard); - Ipv4_address_prefix const &local_intf = local_domain.ip_config().interface; + Ipv4_address_prefix const &local_intf = local_domain.ip_config().interface(); if (ip.more_fragments() || ip.fragment_offset() != 0) { @@ -1350,7 +1350,7 @@ void Interface::_handle_arp_reply(Ethernet_frame ð, destroy(waiter.src()._alloc, &waiter); } } - Ipv4_address_prefix const &local_intf = local_domain.ip_config().interface; + Ipv4_address_prefix const &local_intf = local_domain.ip_config().interface(); if (local_intf.prefix_matches(arp.dst_ip()) && arp.dst_ip() != local_intf.address) { @@ -1400,7 +1400,7 @@ void Interface::_handle_arp_request(Ethernet_frame ð, Domain &local_domain) { Ipv4_config const &local_ip_cfg = local_domain.ip_config(); - Ipv4_address_prefix const &local_intf = local_ip_cfg.interface; + Ipv4_address_prefix const &local_intf = local_ip_cfg.interface(); if (local_intf.prefix_matches(arp.dst_ip())) { /* ARP request for an IP local to the domain's subnet */ @@ -1429,7 +1429,7 @@ void Interface::_handle_arp_request(Ethernet_frame ð, } else { /* ARP request for an IP foreign to the domain's subnet */ - if (local_ip_cfg.gateway_valid) { + if (local_ip_cfg.gateway_valid()) { /* leave request up to the gateway of the domain */ throw Drop_packet("leave ARP request up to gateway"); @@ -1542,7 +1542,7 @@ void Interface::_handle_eth(Ethernet_frame ð, Packet_descriptor const &pkt, Domain &local_domain) { - if (local_domain.ip_config().valid) { + if (local_domain.ip_config().valid()) { switch (eth.type()) { case Ethernet_frame::Type::ARP: _handle_arp(eth, size_guard, local_domain); break; @@ -1801,7 +1801,7 @@ void Interface::_update_link_check_nat(Link &link, return; } try { - if (link.server().dst_ip() != new_srv_dom.ip_config().interface.address) { + if (link.server().dst_ip() != new_srv_dom.ip_config().interface().address) { _dismiss_link_log(link, "NAT IP"); throw Dismiss_link(); } @@ -2125,7 +2125,7 @@ void Interface::handle_config_3() return; } /* if there was/is no IP config, there is nothing more to update */ - if (!new_domain.ip_config().valid) { + if (!new_domain.ip_config().valid()) { return; } /* update state objects */ diff --git a/repos/os/src/server/nic_router/ipv4_config.cc b/repos/os/src/server/nic_router/ipv4_config.cc index 8bf7d6eeab..89794bf831 100644 --- a/repos/os/src/server/nic_router/ipv4_config.cc +++ b/repos/os/src/server/nic_router/ipv4_config.cc @@ -23,30 +23,30 @@ using namespace Net; Ipv4_config::Ipv4_config(Allocator &alloc) : - alloc { alloc }, - interface { }, - gateway { } + _alloc { alloc }, + _interface { }, + _gateway { } { } Ipv4_config::Ipv4_config(Xml_node const &domain_node, Allocator &alloc) : - alloc { alloc }, - interface { domain_node.attribute_value("interface", Ipv4_address_prefix()) }, - gateway { domain_node.attribute_value("gateway", Ipv4_address()) } + _alloc { alloc }, + _interface { domain_node.attribute_value("interface", Ipv4_address_prefix()) }, + _gateway { domain_node.attribute_value("gateway", Ipv4_address()) } { } Ipv4_config::Ipv4_config(Ipv4_config const &ip_config, Allocator &alloc) : - alloc { alloc }, - interface { ip_config.interface }, - gateway { ip_config.gateway } + _alloc { alloc }, + _interface { ip_config._interface }, + _gateway { ip_config._gateway } { - ip_config.dns_servers.for_each([&] (Dns_server const &dns_server) { - dns_servers.insert_as_tail( + ip_config._dns_servers.for_each([&] (Dns_server const &dns_server) { + _dns_servers.insert_as_tail( *new (alloc) Dns_server(dns_server.ip())); }); } @@ -55,10 +55,10 @@ Ipv4_config::Ipv4_config(Ipv4_config const &ip_config, Ipv4_config::Ipv4_config(Dhcp_packet &dhcp_ack, Allocator &alloc) : - alloc { alloc }, - interface { dhcp_ack.yiaddr(), - dhcp_ipv4_option(dhcp_ack) }, - gateway { dhcp_ipv4_option(dhcp_ack) } + _alloc { alloc }, + _interface { dhcp_ack.yiaddr(), + dhcp_ipv4_option(dhcp_ack) }, + _gateway { dhcp_ipv4_option(dhcp_ack) } { try { @@ -66,7 +66,7 @@ Ipv4_config::Ipv4_config(Dhcp_packet &dhcp_ack, dhcp_ack.option() }; dns_server.for_each_address([&] (Ipv4_address const &addr) { - dns_servers.insert_as_tail(*new (alloc) Dns_server(addr)); + _dns_servers.insert_as_tail(*new (alloc) Dns_server(addr)); }); } catch (Dhcp_packet::Option_not_found) { } @@ -75,20 +75,30 @@ Ipv4_config::Ipv4_config(Dhcp_packet &dhcp_ack, Ipv4_config::~Ipv4_config() { - dns_servers.destroy_each(alloc); + _dns_servers.destroy_each(_alloc); } void Ipv4_config::print(Output &output) const { - if (valid) { + if (_valid) { - Genode::print(output, "interface ", interface, ", gateway ", gateway, - " P2P ", point_to_point); + Genode::print(output, "interface ", _interface, ", gateway ", _gateway, + ", P2P ", _point_to_point); for_each_dns_server([&] (Dns_server const &dns_server) { Genode::print(output, ", DNS server ", dns_server.ip()); }); + } else if (_interface_valid || _gateway_valid || !_dns_servers.empty()) { + + Genode::print(output, "malformed (interface ", _interface, + ", gateway ", _gateway, ", P2P ", _point_to_point); + + for_each_dns_server([&] (Dns_server const &dns_server) { + Genode::print(output, ", DNS server ", dns_server.ip()); }); + + Genode::print(output, ")"); + } else { Genode::print(output, "none"); diff --git a/repos/os/src/server/nic_router/ipv4_config.h b/repos/os/src/server/nic_router/ipv4_config.h index 30547c729f..4b5725bf9b 100644 --- a/repos/os/src/server/nic_router/ipv4_config.h +++ b/repos/os/src/server/nic_router/ipv4_config.h @@ -24,56 +24,70 @@ namespace Net { class Ipv4_config; } -struct Net::Ipv4_config +class Net::Ipv4_config { - Genode::Allocator &alloc; - Ipv4_address_prefix const interface; - bool const interface_valid { interface.valid() }; - Ipv4_address const gateway; - bool const gateway_valid { gateway.valid() }; - bool const point_to_point { gateway_valid && - interface_valid && - interface.prefix == 32 }; - Net::List dns_servers { }; - bool const valid { point_to_point || - (interface_valid && - (!gateway_valid || - interface.prefix_matches(gateway))) }; + private: - Ipv4_config(Net::Dhcp_packet &dhcp_ack, - Genode::Allocator &alloc); + Genode::Allocator &_alloc; + Ipv4_address_prefix const _interface; + bool const _interface_valid { _interface.valid() }; + Ipv4_address const _gateway; + bool const _gateway_valid { _gateway.valid() }; + bool const _point_to_point { _gateway_valid && + _interface_valid && + _interface.prefix == 32 }; + Net::List _dns_servers { }; + bool const _valid { _point_to_point || + (_interface_valid && + (!_gateway_valid || + _interface.prefix_matches(_gateway))) }; - Ipv4_config(Genode::Xml_node const &domain_node, - Genode::Allocator &alloc); + public: - Ipv4_config(Ipv4_config const &ip_config, - Genode::Allocator &alloc); + Ipv4_config(Net::Dhcp_packet &dhcp_ack, + Genode::Allocator &alloc); - Ipv4_config(Genode::Allocator &alloc); + Ipv4_config(Genode::Xml_node const &domain_node, + Genode::Allocator &alloc); - ~Ipv4_config(); + Ipv4_config(Ipv4_config const &ip_config, + Genode::Allocator &alloc); - bool operator != (Ipv4_config const &other) const - { - return interface != other.interface || - gateway != other.gateway || - !dns_servers.equal_to(other.dns_servers); - } + Ipv4_config(Genode::Allocator &alloc); - template - void for_each_dns_server(FUNC && functor) const - { - dns_servers.for_each([&] (Dns_server const &dns_server) { - functor(dns_server); - }); - } + ~Ipv4_config(); + + bool operator != (Ipv4_config const &other) const + { + return _interface != other._interface || + _gateway != other._gateway || + !_dns_servers.equal_to(other._dns_servers); + } + + template + void for_each_dns_server(FUNC && functor) const + { + _dns_servers.for_each([&] (Dns_server const &dns_server) { + functor(dns_server); + }); + } - /********* - ** log ** - *********/ + /********* + ** log ** + *********/ - void print(Genode::Output &output) const; + void print(Genode::Output &output) const; + + + /*************** + ** Accessors ** + ***************/ + + bool valid() const { return _valid; } + Ipv4_address_prefix const &interface() const { return _interface; } + Ipv4_address const &gateway() const { return _gateway; } + bool gateway_valid() const { return _gateway_valid; } }; #endif /* _IPV4_CONFIG_H_ */