search

rust-embedded-community / embedded-nal

An Embedded Network Abstraction Layer
Apache License 2.0
177 stars 25 forks source link

Retrieve multiple IP addresses when resolving a hostname. #58

Closed lachlansneff closed 3 years ago

lachlansneff commented 3 years ago

This PR changes the Dns::get_host_by_name function to Dns::get_hosts_by_name and makes it return an iterator.

lachlansneff commented 3 years ago

I also want to point out it doesn't make sense to return nb::Result because there's no sensible way to resume a lookup if it returns WouldBlock.

ryan-summers commented 3 years ago

cc @MathiasKoch as I believe he's the only one using the DNS traits so far (although I intend to leverage them in the coming weeks).

MathiasKoch commented 3 years ago

I do use them yeah, but they are in no way optimal or ergonomic to use currently, so i am open to anything that improves the current design flaws.

I would like to see some kind of reference implementation as well though?

Also, if the case is that nb doesn't make sense, which i tend to agree with, perhaps we should just remove it?

Sympatron commented 3 years ago

It seems to be difficult to implement something this trait, because the Iterator would either need to keep borrowing the stack or own the list of ip addresses, which can be difficult without alloc (you could still only return a predefined maximum number of addresses). I am not entirely sure about this, but I can't see an easy way to solve this. maybe passing a &mut [IpAddr] would be simpler.

Also, if the case is that nb doesn't make sense, which i tend to agree with, perhaps we should just remove it?

I think embedded-hal is also getting rid of nb, so I would be in favor of that, too. To my understanding they are going to provide different traits for synchronous, async and future based APIs.

Some kind of asynchronous API is really important for a network stack though, because operations can take a really long time.

lachlansneff commented 3 years ago

If we use GATs, we could return an Iterator that borrows self.

ryan-summers commented 3 years ago

GATs are not stable, so we cannot yet use them.

lachlansneff commented 3 years ago

That's true. To take a slice, it should really take &mut [MaybeUninit<IpAddr>] and return &[IpAddr].

ryan-summers commented 3 years ago

That's true. To take a slice, it should really take &mut [MaybeUninit<IpAddr>] and return &[IpAddr].

Can you please develop a reference implementation to show how these changes would be used? I do not want to merge changes that we aren't sure will work how we'd like.

lachlansneff commented 3 years ago

Sure, I'll hook it up to nrfxlib.

lachlansneff commented 3 years ago

@ryan-summers Here's an example implementation: https://github.com/lachlansneff/nrfxlib/blob/109392c6ad2d6d85ab93330ea399a2df2be69e49/src/tcp.rs#L281-L358

One thing I'm unsure of is the intended implementation target of the trait. Is it intended to be implemented on the same type as TcpStack or UdpStack?

ryan-summers commented 3 years ago

One thing I'm unsure of is the intended implementation target of the trait. Is it intended to be implemented on the same type as TcpStack or UdpStack?

Generally speaking, yes. There's a single "Network stack" that provides a socket-like API to the application (e.g. smoltcp or the w5500). Thus, you're allowed to have any internal storage for the implementing structure as well. If you want to see an example, take a look at https://github.com/quartiq/smoltcp-nal (although this doesn't have DNS yet)

lachlansneff commented 3 years ago

I came up with an example implementation that returns an iterator that can borrow self without GATs. https://github.com/lachlansneff/nrfxlib/blob/9c814bb69f2cd7cc77ca58dbe095cb6c43b688cc/src/tcp.rs#L267-L379

Apologies for the weird indentation.

lachlansneff commented 3 years ago

Another option is to call a callback for each IP address returned.

lachlansneff commented 3 years ago

I also want to point out that because the dns methods take &mut self, any returned item that borrows self means that connect can't be called unless the addresses are first moved somewhere else, which defeats the whole point. Therefore, I think it's reasonable to return an owned iterator.

lachlansneff commented 3 years ago

I'm going to close this PR and open an issue about changing the embedded-nal API to more closely resemble std::net.

Sympatron commented 3 years ago

I came up with an example implementation that returns an iterator that can borrow self without GATs. https://github.com/lachlansneff/nrfxlib/blob/9c814bb69f2cd7cc77ca58dbe095cb6c43b688cc/src/tcp.rs#L267-L379

Apologies for the weird indentation.

This only works, because the API you are using internally calls malloc/free. Without a heap it's not possible.