DHCPv4 client #186

astro · 2018-04-05T21:33:12Z

Hi

This is an attempt at creating a DHCP client for IPv4. Please give feedback.

Cc: @phil-opp, this PR is uses the work merged in #75.

I've taken a completely different approach compared to #63. UDP sockets don't allow us to send and receive on unspecified/broadcast IPv4 addresses. Therefore IPv4/UDP is parsed/serialized on top of a RawSocket.

~~There are still two problems with RawSocketBuffer:~~

~~How do I let the user provide storage? I have tried but failed to accept that as parameters in Client::new().~~ I've found the proper lifetime relation by now.
The RawSocket is getting stuck with Error::Exhausted when contig_window is too small for the DHCP egress packet. Somehow it appears to behave like sent packets don't get dequeued from the tx_buffer. Is that known? What is my code doing wrong regarding RawSocket? See RingBuffer contiguous memory optimization #187

m-labs-homu · 2018-05-07T16:00:39Z

☔ The latest upstream changes (presumably 6c36376) made this pull request unmergeable. Please resolve the merge conflicts.

birkenfeld · 2018-06-28T12:34:10Z

I'm using this branch (rebased on master, which was mostly painless), and it works fine for me. The only thing I changed was to increase the discover timeout since my DHCP server was too slow in sending replies, and the sequence number of the reply never matched the last request.

whitequark · 2018-07-11T01:15:47Z

I'm sorry for the delay in review.

Can you please extract the changes to wire/dhcp.rs first so we can merge that (I don't see anything wrong on the first glance) and then discuss the rest? I think your overall approach is fine but some API consistency changes would be nice.

robomancer-or · 2018-07-25T22:16:32Z

Would this PR be a suitable starting point for implementing PXE for no_std environments? If so, do we have a estimate of when it may land?

m-labs-homu · 2018-07-31T11:42:54Z

☔ The latest upstream changes (presumably 1fbcfb0) made this pull request unmergeable. Please resolve the merge conflicts.

robomancer-or

Response inline, TLDR, as implemented other uses of DHCP are impossible.

robomancer-or · 2018-07-31T15:13:14Z

src/dhcp/clientv4.rs

+
+    /// Process incoming packets on the contained RawSocket, and send
+    /// DHCP requests when timeouts are ready.
+    pub fn poll<DeviceT: for<'d> Device<'d>>(&mut self, iface: &mut Interface<DeviceT>, sockets: &mut SocketSet, now: Instant) -> Result<()> {


Not returning anything here means that this code can't be used for other types of host configuration (notably: PXE, for network booting). DHCP is designed to do more than just networking parameters. Is there some reason not to return some representation of the response packet?

I have given it a try to return the DhcpRepr<'a> but that is really cumbersome as it is bound to the lifetime of the packet received by raw_socket.recv() in this function. Check my branch dhcp-return if you would like to work on that.

Proposals are welcome. This is just a as-simple-as-possible implementation.

Step one I'll get a proof of concept working locally (otherwise my proposal is likely to miss some important details). I'll be back.

No, I got put onto a different thing... :-/

Did you say here that there is genuine commercial interest in smoltcp and embedded Rust?

Why wouldn't there be?

m-labs-homu · 2018-08-01T02:36:45Z

☔ The latest upstream changes (presumably 58a5473) made this pull request unmergeable. Please resolve the merge conflicts.

astro · 2018-08-02T16:44:51Z

Rebased this one.

therealprof · 2018-11-06T12:30:28Z

@whitequark Is there anything we (I) can do to move this forward?

hartytp · 2018-12-29T17:00:17Z

@whitequark what's the status of this PR?

phil-opp · 2019-01-21T17:48:52Z

@astro I just tried this PR and it works great overall! Thanks for your work!

There seems to be an issue with the renew code. It throws a Truncated error for me:

Backtrace

#0  _$LT$smoltcp..wire..ipv4..Packet$LT$T$GT$$GT$::check_len::h704ea3dba9d0fe4d (self=0x2004ddcc) at /home/philipp/.cargo/git/checkouts/smoltcp-ee37870a12a4cf78/d834b20/src/wire/ipv4.rs:283
#1  0x0800ebd4 in _$LT$smoltcp..wire..ipv4..Packet$LT$T$GT$$GT$::new_checked::h81e7878ad05d032a (buffer=0x2004e03c) at /home/philipp/.cargo/git/checkouts/smoltcp-ee37870a12a4cf78/d834b20/src/wire/ipv4.rs:260
#2  0x080111ea in smoltcp::dhcp::clientv4::send_packet::heb7cfda033ccaf55 (iface=0x200008f0, raw_socket=0x200061a8, endpoint=0x2004e354, dhcp_repr=0x2004e368, checksum_caps=0x2004e4b0)
    at /home/philipp/.cargo/git/checkouts/smoltcp-ee37870a12a4cf78/d834b20/src/dhcp/clientv4.rs:412
#3  0x08011b76 in smoltcp::dhcp::clientv4::Client::egress::h61ebcdda26ac545a (self=0x20000980, iface=0x200008f0, raw_socket=0x200061a8, checksum_caps=0x2004e4b0, now=...)
    at /home/philipp/.cargo/git/checkouts/smoltcp-ee37870a12a4cf78/d834b20/src/dhcp/clientv4.rs:319
#4  0x08011452 in smoltcp::dhcp::clientv4::Client::poll::hf5e5a434e6960c04 (self=0x20000980, iface=0x200008f0, sockets=0x200009d0, now=...)
    at /home/philipp/.cargo/git/checkouts/smoltcp-ee37870a12a4cf78/d834b20/src/dhcp/clientv4.rs:128

Seems like the renew packet fails the Ipv4::check_len() test because len < self.total_len().

astro · 2019-02-03T22:36:24Z

I wonder if we should temporarily fork smoltcp…

whitequark · 2019-02-03T22:37:47Z

I'll take a look at this soon.

astro · 2019-02-04T20:59:25Z

Rebased and added logging.

There seems to be an issue with the renew code. It throws a Truncated error for me:

Fixed this is in bc498fa and subsequently corrected some fields in the Renew packets.

whitequark · 2019-04-12T06:43:31Z

As far as I understand there are outstanding issues, so I'm not merging this yet.

therealprof · 2019-04-12T06:49:46Z

@whitequark It's unclear whether there's a problem or not, it sounds as if @phil-opp might be using it wrong.

Merging "some" version would allow more people to use it and test. I'd never assume a new implementation of a messy protocol like DHCP is going to be perfect from day one but if it is not landed at some point it'll never have a chance to become perfect.

whitequark · 2019-04-12T06:50:46Z

OK, so at least this'll need to be rebased.

oli-obk · 2019-04-12T06:53:51Z

I've been experimenting with this and everything but assigning the new IP address seems to work. Even not using the DHCPv4 client I am unable to change the IP address after building an EthernetInterface. We might indeed be using the API wrong, so please don't block this on us if it works for you, I'll figure it out aftewards.

astro · 2019-04-13T13:20:18Z

OK, so at least this'll need to be rebased.

Rebased!

Note that #276 added more dst_addr checks to process_ipv4() which were removed in this PR.

m-labs-homu · 2019-04-14T18:08:27Z

☔ The latest upstream changes (presumably 5334b93) made this pull request unmergeable. Please resolve the merge conflicts.

astro · 2019-04-20T19:40:44Z

Rebased again.

Note that #276 added more dst_addr checks to process_ipv4() which were removed in this PR.

To explain this again: We cannot filter on bound IP addresses as the DHCP responses are targeted at IP addresses that are yet to be bound. Using DHCP would depend on always running in any_ip mode. This is a consequence of moving from RawSocket to UdpSocket, as was desired in code review. Removal of the filtering code also means we would depend on running with a unique mac addr on a switched ethernet link.

I suggest that we revert back to using a RawSocket for the DhcpClient, like it is being done in other implementations on other stacks. That way we can keep dst address filtering. What do you think?

whitequark · 2019-04-21T02:39:54Z

I suggest that we revert back to using a RawSocket for the DhcpClient, like it is being done in other implementations on other stacks. That way we can keep dst address filtering. What do you think?

I think this is unfortunate but probably a reasonable way forward. Sorry for making you do the work twice.

astro · 2019-04-28T13:58:20Z

Switched back to raw sockets, rebased, and squashed.

Please review my changes to src/iface/ethernet.rs. I had to prioritize raw sockets so that UDP won't respond with ICMP port-unreachable even when a packet has been handled by a raw socket.

oli-obk · 2019-04-29T19:00:28Z

src/dhcp/clientv4.rs

+    }
+
+    fn egress<DeviceT: for<'d> Device<'d>>(&mut self, iface: &mut Interface<DeviceT>, raw_socket: &mut RawSocket, checksum_caps: &ChecksumCapabilities, now: Instant) -> Result<Option<Config>> {
+        println!("DHCP egress");


can you remove these debugging statements? They are hard to use on no_std ;)

Ooops, thanks for the find!

Nice talk on saturday, btw.

m-labs-homu · 2019-05-01T02:23:06Z

☔ The latest upstream changes (presumably e7e267f) made this pull request unmergeable. Please resolve the merge conflicts.

astro · 2019-05-08T21:58:42Z

After #292, #293/#295, rebased once again.

whitequark · 2019-05-13T16:37:32Z

I think this is basically okay, and there is no reason to have it drag on any longer even if it's imperfect in minor ways. I apologize that this took so long in the first place.

@m-labs-homu r+

m-labs-homu · 2019-05-13T16:37:36Z

📌 Commit 14695c1 has been approved by whitequark

m-labs-homu · 2019-05-13T16:37:48Z

⌛ Testing commit 14695c1 with merge 657c9aa...

m-labs-homu · 2019-05-13T16:49:23Z

☀️ Test successful - status-travis
Approved by: whitequark
Pushing 657c9aa to master...

astro · 2019-05-13T19:59:46Z

Wow, after 13 months!

it's imperfect in minor ways

I agree. It's meant to be a basic implementation to get DHCP capabilities started.

I don't believe in code ownership, so anyone is invited to overhaul this code. Mention @astro in the issues/PRs so that I know where I can help.

astro mentioned this pull request Apr 9, 2018

RingBuffer contiguous memory optimization #187

Merged

astro force-pushed the dhcp branch from 34bbc84 to 3375fb9 Compare April 9, 2018 22:54

phil-opp mentioned this pull request Apr 20, 2018

DHCP support embed-rs/stm32f7-discovery#68

Open

jordens mentioned this pull request Jul 4, 2018

core device DHCP support m-labs/artiq#1085

Closed

astro force-pushed the dhcp branch from f56bc45 to 0acca35 Compare July 21, 2018 20:25

astro mentioned this pull request Jul 21, 2018

Add a few DHCP options #255

Closed

robomancer-or reviewed Jul 31, 2018

View reviewed changes

astro force-pushed the dhcp branch 3 times, most recently from 5b82cc8 to cd9773d Compare July 31, 2018 20:28

astro force-pushed the dhcp branch from cd9773d to c331ba6 Compare August 2, 2018 00:29

astro force-pushed the dhcp branch from c331ba6 to ae2cf0c Compare August 13, 2018 21:12

astro force-pushed the dhcp branch from 15797ef to 0bf78eb Compare August 22, 2018 23:19

astro force-pushed the dhcp branch from 0bf78eb to b51b3d6 Compare October 3, 2018 17:37

astro force-pushed the dhcp branch from b51b3d6 to d834b20 Compare November 6, 2018 16:31

phil-opp mentioned this pull request Feb 1, 2019

DHCP renew packet causes Truncated error embed-rs/stm32f7-discovery#77

Open

astro force-pushed the dhcp branch from d834b20 to 51caedf Compare February 4, 2019 20:52

astro force-pushed the dhcp branch from 8333045 to 3c3cc26 Compare April 13, 2019 13:18

astro force-pushed the dhcp branch from 3c3cc26 to 9329391 Compare April 20, 2019 19:23

astro force-pushed the dhcp branch 2 times, most recently from f64eb47 to 2f7efb7 Compare April 28, 2019 13:56

oli-obk reviewed Apr 29, 2019

View reviewed changes

astro force-pushed the dhcp branch from 2f7efb7 to d8b6be3 Compare April 29, 2019 20:47

This was referenced Apr 30, 2019

test_icmp_reply_size: fix for ipv4-only case #292

Closed

Suppress ICMP error replies for packets that are accepted by raw sockets #293

Closed

Implement DHCPv4 client + example.

14695c1

astro force-pushed the dhcp branch from d8b6be3 to 14695c1 Compare May 8, 2019 21:56

whitequark approved these changes May 13, 2019

View reviewed changes

m-labs-homu closed this in 657c9aa May 13, 2019

phil-opp mentioned this pull request Mar 12, 2020

DHCP support #63

Closed

DHCPv4 client #186

DHCPv4 client #186

Conversation

astro commented Apr 5, 2018 • edited Loading

m-labs-homu commented May 7, 2018

birkenfeld commented Jun 28, 2018

whitequark commented Jul 11, 2018 • edited Loading

robomancer-or commented Jul 25, 2018

m-labs-homu commented Jul 31, 2018

robomancer-or left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-labs-homu commented Aug 1, 2018

astro commented Aug 2, 2018

therealprof commented Nov 6, 2018

hartytp commented Dec 29, 2018

phil-opp commented Jan 21, 2019

astro commented Feb 3, 2019

whitequark commented Feb 3, 2019

astro commented Feb 4, 2019

whitequark commented Apr 12, 2019

therealprof commented Apr 12, 2019

whitequark commented Apr 12, 2019

oli-obk commented Apr 12, 2019

astro commented Apr 13, 2019

m-labs-homu commented Apr 14, 2019

astro commented Apr 20, 2019

whitequark commented Apr 21, 2019

astro commented Apr 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-labs-homu commented May 1, 2019

astro commented May 8, 2019

whitequark commented May 13, 2019

m-labs-homu commented May 13, 2019

m-labs-homu commented May 13, 2019

m-labs-homu commented May 13, 2019

astro commented May 13, 2019

astro commented Apr 5, 2018 •

edited

Loading

whitequark commented Jul 11, 2018 •

edited

Loading