I forgot to mention that I tested this patch on both VMware with InfoBlox DHCP server and EC2.
Sent from my iPhone > On Mar 22, 2017, at 09:18, Nadav Har'El <[email protected]> wrote: > > Thanks. Looks good to me. Justin, can you please also have a look? > > > -- > Nadav Har'El > [email protected] > >> On Wed, Mar 22, 2017 at 2:57 PM, Waldemar Kozaczuk <[email protected]> >> wrote: >> From: wkozaczuk <[email protected]> >> >> The patch addressing issue #816 changed dhcp_mbuf::compose_request() method >> to >> make it unicast DHCPREQUEST message to a DHCP server. This is incorrect per >> RCF-2131 >> which requires DHCP clients to broadcast messages until IP address is bound >> which >> is a case during RENEWING phase but not SELECTING one. Now the >> dhcp_mbuf::compose_request() >> takes extra enum parameter which specifies which phase (per page 34 of >> https://www.ietf.org/rfc/rfc2131.txt) >> the message is being composed for. >> >> Fixes #864 >> >> Signed-off-by: Waldemar Kozaczuk <[email protected]> >> --- >> core/dhcp.cc | 55 >> ++++++++++++++++++++++++++++++++++++++++++++++------- >> include/osv/dhcp.hh | 10 +++++++++- >> 2 files changed, 57 insertions(+), 8 deletions(-) >> >> diff --git a/core/dhcp.cc b/core/dhcp.cc >> index 1b90adb..12f27bb 100644 >> --- a/core/dhcp.cc >> +++ b/core/dhcp.cc >> @@ -81,11 +81,26 @@ namespace dhcp { >> >> const char * dhcp_options_magic = "\x63\x82\x53\x63"; >> >> + static std::map<dhcp_message_type,const char*> >> dhcp_message_type_name_by_id = { >> + {DHCP_MT_DISCOVER, "DHCPDISCOVER"}, >> + {DHCP_MT_OFFER, "DHCPOFFER"}, >> + {DHCP_MT_REQUEST, "DHCPREQUEST"}, >> + {DHCP_MT_DECLINE, "DHCPDECLINE"}, >> + {DHCP_MT_ACK, "DHCPACK"}, >> + {DHCP_MT_NAK, "DHCPNAK"}, >> + {DHCP_MT_RELEASE, "DHCPRELEASE"}, >> + {DHCP_MT_INFORM, "DHCPINFORM"}, >> + {DHCP_MT_LEASEQUERY, "DHCPLEASEQUERY"}, >> + {DHCP_MT_LEASEUNASSIGNED, "DHCPLEASEUNASSIGNED"}, >> + {DHCP_MT_LEASEUNKNOWN, "DHCPLEASEUNKNOWN"}, >> + {DHCP_MT_LEASEACTIVE, "DHCPLEASEACTIVE"}, >> + {DHCP_MT_INVALID, "DHCPINVALID"} >> + }; >> + >> >> /////////////////////////////////////////////////////////////////////////// >> >> bool dhcp_socket::dhcp_send(dhcp_mbuf& packet) >> { >> - >> struct bsd_sockaddr dst = {}; >> struct mbuf *m = packet.get(); >> >> @@ -205,7 +220,8 @@ namespace dhcp { >> void dhcp_mbuf::compose_request(struct ifnet* ifp, >> u32 xid, >> ip::address_v4 yip, >> - ip::address_v4 sip) >> + ip::address_v4 sip, >> + dhcp_request_packet_type >> request_packet_type) >> { >> size_t dhcp_len = sizeof(struct dhcp_packet); >> struct dhcp_packet* pkt = pdhcp(); >> @@ -222,7 +238,11 @@ namespace dhcp { >> 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); >> + if(request_packet_type == DHCP_REQUEST_RENEWING || >> request_packet_type == DHCP_REQUEST_REBINDING) { >> + // ciaddr should only be set to if RENEWING or REBINDING >> + // See pages 21 and 30-31 in >> https://www.ietf.org/rfc/rfc2131.txt >> + memcpy(&pkt->ciaddr.s_addr, &yip_n, 4); >> + } >> >> // Options >> u8* options_start = reinterpret_cast<u8*>(pkt+1); >> @@ -244,7 +264,14 @@ namespace dhcp { >> *options++ = DHCP_OPTION_END; >> >> dhcp_len += options - options_start; >> - build_udp_ip_headers(dhcp_len, yip_n, sip_n); >> + >> + // See page 33 in https://www.ietf.org/rfc/rfc2131.txt >> + if(request_packet_type == DHCP_REQUEST_RENEWING) { >> + build_udp_ip_headers(dhcp_len, yip_n, sip_n); >> + } >> + else { >> + build_udp_ip_headers(dhcp_len, INADDR_ANY, INADDR_BROADCAST); >> + } >> } >> >> void dhcp_mbuf::compose_release(struct ifnet* ifp, >> @@ -420,6 +447,9 @@ namespace dhcp { >> options += op_len; >> } >> >> + dhcp_i( "Received %s message from DHCP server: %s regarding >> offerred IP address: %s", >> + dhcp_message_type_name_by_id[_message_type], >> _dhcp_server_ip.to_string().c_str(), >> + _your_ip.to_string().c_str()); >> return true; >> } >> >> @@ -519,6 +549,7 @@ namespace dhcp { >> // Save transaction id & send >> _xid = dm.get_xid(); >> _client_addr = _server_addr = ipv4_zero; >> + dhcp_i( "Broadcasting DHCPDISCOVER message with xid: [%d]",_xid); >> _sock->dhcp_send(dm); >> } >> >> @@ -533,6 +564,8 @@ namespace dhcp { >> >> // Save transaction id & send >> _xid = dm.get_xid(); >> + dhcp_i( "Unicasting DHCPRELEASE message with xid: [%d] from client: >> %s to server: %s", >> + _xid, _client_addr.to_string().c_str(), >> _server_addr.to_string().c_str()); >> _sock->dhcp_send(dm); >> // IP and routes have to be removed >> osv::stop_if(_ifp->if_xname, _client_addr.to_string().c_str()); >> @@ -553,9 +586,13 @@ namespace dhcp { >> _xid = rand(); >> dm.compose_request(_ifp, >> _xid, >> - _client_addr, _server_addr); >> + _client_addr, >> + _server_addr, >> + dhcp_mbuf::DHCP_REQUEST_RENEWING); >> >> // Send >> + dhcp_i( "Unicasting DHCPREQUEST message with xid: [%d] from client: >> %s to server: %s in order to RENEW lease of: %s", >> + _xid, _client_addr.to_string().c_str(), >> _server_addr.to_string().c_str(), _client_addr.to_string().c_str()); >> _sock->dhcp_send(dm); >> } >> >> @@ -600,14 +637,18 @@ namespace dhcp { >> dm_req.compose_request(_ifp, >> _xid, >> dm.get_your_ip(), >> - dm.get_dhcp_server_ip()); >> + dm.get_dhcp_server_ip(), >> + dhcp_mbuf::DHCP_REQUEST_SELECTING); >> + dhcp_i( "Broadcasting DHCPREQUEST message with xid: [%d] to SELECT >> offered IP: %s", >> + _xid, dm.get_your_ip().to_string().c_str()); >> _sock->dhcp_send(dm_req); >> } >> >> void dhcp_interface_state::state_request(dhcp_mbuf &dm) >> { >> if (dm.get_message_type() == DHCP_MT_ACK) { >> - dhcp_i("Server acknowledged IP for interface %s", >> _ifp->if_xname); >> + dhcp_i("Server acknowledged IP %s for interface %s with time to >> lease in seconds: %d", >> + dm.get_your_ip().to_string().c_str(), _ifp->if_xname, >> dm.get_lease_time_sec()); >> _state = DHCP_ACKNOWLEDGE; >> _client_addr = dm.get_your_ip(); >> _server_addr = dm.get_dhcp_server_ip(); >> diff --git a/include/osv/dhcp.hh b/include/osv/dhcp.hh >> index 3db6df8..6797b7f 100644 >> --- a/include/osv/dhcp.hh >> +++ b/include/osv/dhcp.hh >> @@ -117,6 +117,13 @@ namespace dhcp { >> DHCP_REPLY = 2, >> }; >> >> + enum dhcp_request_packet_type { >> + DHCP_REQUEST_INIT_REBOOT = 1, >> + DHCP_REQUEST_SELECTING = 2, >> + DHCP_REQUEST_RENEWING = 3, >> + DHCP_REQUEST_REBINDING = 4, >> + }; >> + >> dhcp_mbuf(bool attached = true, struct mbuf* m = nullptr); >> ~dhcp_mbuf(); >> >> @@ -129,7 +136,8 @@ namespace dhcp { >> void compose_request(struct ifnet* ifp, >> u32 xid, >> boost::asio::ip::address_v4 yip, >> - boost::asio::ip::address_v4 sip); >> + boost::asio::ip::address_v4 sip, >> + dhcp_request_packet_type request_packet_type); >> void compose_release(struct ifnet* ifp, >> boost::asio::ip::address_v4 yip, >> boost::asio::ip::address_v4 sip); >> -- >> 2.7.4 >> >> -- >> 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 [email protected]. >> 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 [email protected]. For more options, visit https://groups.google.com/d/optout.
