Thanks, looks good. I have one nitpick below, but please consider it for a
followup patch (and also the millisecond-sleep issue).
I'm adding "Fixes #720", as it really does, right?
On Tue, Nov 22, 2016 at 5:52 PM, Justin Cinkelj <justin.cink...@xlab.si>
wrote:
> The release packet needs to contain client and server IP, so they are
> stored to dhcp_interface_state when DHCP ACK is received.
> During shutdown, each interface with DHCP address releases its DHCP IP.
>
> The code does not properly wait for UDP packet to be actually put
> on wire. Just a simple usleep is used to hopefully wait long enough.
> See also #557 and #720.
>
> Signed-off-by: Justin Cinkelj <justin.cink...@xlab.si>
> ---
> core/dhcp.cc | 74 ++++++++++++++++++++++++++++++
> +++++++++++++++++++++++
> core/shutdown.cc | 3 +++
> include/osv/dhcp.hh | 12 ++++++++-
> 3 files changed, 88 insertions(+), 1 deletion(-)
>
> diff --git a/core/dhcp.cc b/core/dhcp.cc
> index 5bbb421..689a387 100644
> --- a/core/dhcp.cc
> +++ b/core/dhcp.cc
> @@ -45,6 +45,8 @@ u8 requested_options[] = {
> dhcp::DHCP_OPTION_HOSTNAME
> };
>
> +const ip::address_v4 ipv4_zero = ip::address_v4::address_v4::
> from_string("0.0.0.0");
>
Nitpick: I think ip::address_v4 has a default constructor for v4, some
something as simple as
constexpr ip::address_v4 ipv4_zero {};
should also be enough.
Even more simply, instead of naming this constant, just use ... = {} below
instead of ... = ipv4_zero, I think that should work too.
> +
> // Returns whether we hooked the packet
> int dhcp_hook_rx(struct mbuf* m)
> {
> @@ -67,6 +69,12 @@ void dhcp_start(bool wait)
> net_dhcp_worker.init(wait);
> }
>
> +// Send DHCP release, for example at shutdown.
> +void dhcp_release()
> +{
> + net_dhcp_worker.release();
> +}
> +
> namespace dhcp {
>
> const char * dhcp_options_magic = "\x63\x82\x53\x63";
> @@ -234,6 +242,42 @@ namespace dhcp {
> build_udp_ip_headers(dhcp_len);
> }
>
> + void dhcp_mbuf::compose_release(struct ifnet* ifp,
> + ip::address_v4 yip,
> + ip::address_v4 sip)
> + {
> + size_t dhcp_len = sizeof(struct dhcp_packet);
> + struct dhcp_packet* pkt = pdhcp();
> + *pkt = {};
> +
> + // Header
> + pkt->op = BOOTREQUEST;
> + pkt->htype = HTYPE_ETHERNET;
> + pkt->hlen = ETHER_ADDR_LEN;
> + pkt->hops = 0;
> + // Linux dhclient also uses new xid for release.
> + pkt->xid = rand();
> + pkt->secs = 0;
> + pkt->flags = 0;
> + memcpy(pkt->chaddr, IF_LLADDR(ifp), ETHER_ADDR_LEN);
> + ulong yip_n = htonl(yip.to_ulong());
> + ulong sip_n = htonl(sip.to_ulong());
> + memcpy(&pkt->ciaddr.s_addr, &yip_n, 4);
> +
> + // Options
> + u8* options_start = reinterpret_cast<u8*>(pkt+1);
> + u8* options = options_start;
> + memcpy(options, dhcp_options_magic, 4);
> + options += 4;
> +
> + options = add_option(options, DHCP_OPTION_MESSAGE_TYPE, 1,
> DHCP_MT_RELEASE);
> + options = add_option(options, DHCP_OPTION_DHCP_SERVER, 4,
> reinterpret_cast<u8*>(&sip_n));
> + *options++ = DHCP_OPTION_END;
> +
> + dhcp_len += options - options_start;
> + build_udp_ip_headers(dhcp_len);
> + }
> +
> u32 dhcp_mbuf::get_xid()
> {
> return (pdhcp()->xid);
> @@ -450,6 +494,7 @@ namespace dhcp {
> {
> _sock = new dhcp_socket(ifp);
> _xid = 0;
> + _client_addr = _server_addr = ipv4_zero;
> }
>
> dhcp_interface_state::~dhcp_interface_state()
> @@ -470,9 +515,26 @@ namespace dhcp {
>
> // Save transaction id & send
> _xid = dm.get_xid();
> + _client_addr = _server_addr = ipv4_zero;
> _sock->dhcp_send(dm);
> }
>
> + void dhcp_interface_state::release()
> + {
> + // Update state
> + _state = DHCP_INIT;
> +
> + // Compose a dhcp release packet
> + dhcp_mbuf dm(false);
> + dm.compose_release(_ifp, _client_addr, _server_addr);
> +
> + // Save transaction id & send
> + _xid = dm.get_xid();
> + _sock->dhcp_send(dm);
> + // no reply/ack is expected, after send we just forget all old
> state
> + _client_addr = _server_addr = ipv4_zero;
> + }
> +
> void dhcp_interface_state::process_packet(struct mbuf* m)
> {
> dhcp_mbuf dm(true, m);
> @@ -523,6 +585,8 @@ namespace dhcp {
> if (dm.get_message_type() == DHCP_MT_ACK) {
> dhcp_i("Server acknowledged IP for interface %s",
> _ifp->if_xname);
> _state = DHCP_ACKNOWLEDGE;
> + _client_addr = dm.get_your_ip();
> + _server_addr = dm.get_dhcp_server_ip();
>
> // TODO: check that the IP address is not responding with ARP
> // RFC2131 section 3.1.5
> @@ -577,6 +641,7 @@ namespace dhcp {
> // "If the client receives a DHCPNAK message, the client
> restarts the
> // configuration process."
> _state = DHCP_INIT;
> + _client_addr = _server_addr = ipv4_zero;
> discover();
> }
> // FIXME: retry on timeout and restart DORA sequence if it
> timeout a
> @@ -641,6 +706,15 @@ namespace dhcp {
> } while (!_have_ip && wait);
> }
>
> + void dhcp_worker::release()
> + {
> + for (auto &it: _universe) {
> + it.second->release();
> + }
> + // Wait a bit, so hopefully UDP release packets will be actually
> put on wire.
> + usleep(1000);
> + }
> +
> void dhcp_worker::dhcp_worker_fn()
> {
> while (true) {
> diff --git a/core/shutdown.cc b/core/shutdown.cc
> index a9b4329..30b9512 100644
> --- a/core/shutdown.cc
> +++ b/core/shutdown.cc
> @@ -2,6 +2,7 @@
> #include <osv/power.hh>
> #include <osv/debug.hh>
> #include <osv/sched.hh>
> +#include <osv/dhcp.hh>
>
> extern void vfs_exit(void);
>
> @@ -9,6 +10,8 @@ namespace osv {
>
> void shutdown()
> {
> + dhcp_release();
> +
> // The vfs_exit() call below will forcibly unmount the filesystem. If
> any
> // thread is executing code mapped from a file, these threads may
> crash if
> // they happen to run between the call to vfs_exit() and the call to
> diff --git a/include/osv/dhcp.hh b/include/osv/dhcp.hh
> index 92ab11c..55e0b16 100644
> --- a/include/osv/dhcp.hh
> +++ b/include/osv/dhcp.hh
> @@ -22,7 +22,10 @@
> #include <boost/asio/ip/address.hpp>
> #include <boost/asio/ip/address_v4.hpp>
>
> -extern "C" void dhcp_start(bool wait);
> +extern "C" {
> +void dhcp_start(bool wait);
> +void dhcp_release();
> +}
>
> namespace dhcp {
>
> @@ -126,6 +129,9 @@ namespace dhcp {
> u32 xid,
> boost::asio::ip::address_v4 yip,
> boost::asio::ip::address_v4 sip);
> + void compose_release(struct ifnet* ifp,
> + boost::asio::ip::address_v4 yip,
> + boost::asio::ip::address_v4 sip);
>
> /* Decode packet */
> bool is_valid_dhcp();
> @@ -218,6 +224,7 @@ namespace dhcp {
> ~dhcp_interface_state();
>
> void discover();
> + void release();
> void process_packet(struct mbuf*);
> void state_discover(dhcp_mbuf &dm);
> void state_request(dhcp_mbuf &dm);
> @@ -228,6 +235,8 @@ namespace dhcp {
> state _state;
> struct ifnet* _ifp;
> dhcp_socket* _sock;
> + boost::asio::ip::address_v4 _client_addr;
> + boost::asio::ip::address_v4 _server_addr;
>
> // Transaction id
> u32 _xid;
> @@ -242,6 +251,7 @@ namespace dhcp {
>
> // Initializing a state per interface, sends discover packets
> void init(bool wait);
> + void release();
>
> void dhcp_worker_fn();
> void queue_packet(struct mbuf* m);
> --
> 2.5.5
>
> --
> You received this message because you are subscribed to the Google Groups
> "OSv Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to osv-dev+unsubscr...@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>
--
You received this message because you are subscribed to the Google Groups "OSv
Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to osv-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
