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 <n...@scylladb.com> wrote: > > Thanks. Looks good to me. Justin, can you please also have a look? > > > -- > Nadav Har'El > n...@scylladb.com > >> On Wed, Mar 22, 2017 at 2:57 PM, Waldemar Kozaczuk <jwkozac...@gmail.com> >> wrote: >> From: wkozaczuk <jwkozac...@gmail.com> >> >> 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 <jwkozac...@gmail.com> >> --- >> 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 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.
