The patch works OK for me, tested agains dnsmasq.
One possible improvements. From RFC 2131, page 30-31
o DHCPREQUEST generated during SELECTING state:
Client inserts the address of the selected server in 'server
identifier', 'ciaddr' MUST be zero, 'requested IP address MUST be
filled in with the yiaddr value from the chosen DHCPOFFER.
o DHCPREQUEST generated during RENEWING state:
'server identifier' MUST NOT be filled in, 'requested IP address'
option MUST NOT be filled in, 'ciaddr' MUST be filled in with
client's IP address.
I understand "'server identifier'/'requested IP address' MUST NOT be filled in"
as instruction to not include those two DHCP OPTIONS into RENEWING/REBINDING
DHCPREQUEST.
So I tried to add additional change, as an attempt to be more RFC compliant.
After that change, code still works - well, at least against dnsmasq.
diff --git a/core/dhcp.cc b/core/dhcp.cc
index 12f27bb..5d3ec9a 100644
--- a/core/dhcp.cc
+++ b/core/dhcp.cc
@@ -253,12 +253,16 @@ namespace dhcp {
ip::address_v4::bytes_type dhcp_server_ip = sip.to_bytes();
ip::address_v4::bytes_type requested_ip = yip.to_bytes();
options = add_option(options, DHCP_OPTION_MESSAGE_TYPE, 1,
DHCP_MT_REQUEST);
- options = add_option(options, DHCP_OPTION_DHCP_SERVER, 4,
(u8*)&dhcp_server_ip);
+ if(request_packet_type == DHCP_REQUEST_SELECTING) {
+ options = add_option(options, DHCP_OPTION_DHCP_SERVER, 4,
(u8*)&dhcp_server_ip);
+ }
char hostname[256];
if (0 == gethostname(hostname, sizeof(hostname))) {
options = add_option(options, DHCP_OPTION_HOSTNAME,
strlen(hostname), (u8*)hostname);
}
- options = add_option(options, DHCP_OPTION_REQUESTED_ADDRESS, 4,
(u8*)&requested_ip);
+ if(request_packet_type == DHCP_REQUEST_SELECTING) {
+ options = add_option(options, DHCP_OPTION_REQUESTED_ADDRESS, 4,
(u8*)&requested_ip);
+ }
options = add_option(options, DHCP_OPTION_PARAMETER_REQUEST_LIST,
sizeof(requested_options), requested_options);
*options++ = DHCP_OPTION_END;
----- Original Message -----
> From: "Waldek Kozaczuk" <[email protected]>
> To: "Nadav Har'El" <[email protected]>
> Cc: "justinc1" <[email protected]>, "Osv Dev" <[email protected]>
> Sent: Wednesday, March 22, 2017 2:54:43 PM
> Subject: Re: [PATCH] Changed DHCP client logic to properly broadcast
> DHCPREQUEST during
>
> 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.
