nsapi - Add explicit DNS interface to the network socket API #2652

geky · 2016-09-08T17:28:13Z

Currently in the network-socket API, DNS resolution can only be performed implicitly through functions such as TCPSocket::connect, UDPSocket::sendto, and construction of the SocketAddress which is limited, returns no errors, requires an unintuitive stack parameter, and as a constructor does not imply a heavy operation.

Previously, the network-socket API provided gethostbyname in the NetworkInterface, but this was removed when the NetworkStack class was split out and hidden from users.

This pr proposes adding back the gethostbyname function with IPv4 and IPv6 support to the NetworkInterface as well as dns_add_server to allow users to add custom DNS servers and work around limited stacks.

/** Translates a hostname to an IP address
 *
 *  The hostname may be either a domain name or an IP address. If the
 *  hostname is an IP address, no network transactions will be performed.
 *  
 *  If no stack-specific DNS resolution is provided, the hostname
 *  will be resolve using a UDP socket on the stack. 
 *
 *  @param address  Destination for the host SocketAddress
 *  @param host     Hostname to resolve
 *  @return         0 on success, negative error code on failure
 */
int NetworkInterface::gethostbyname(const char *host, SocketAddress *address);

/** Translates a hostname to an IP address with specific version
 *
 *  The hostname may be either a domain name or an IP address. If the
 *  hostname is an IP address, no network transactions will be performed.
 *  
 *  If no stack-specific DNS resolution is provided, the hostname
 *  will be resolve using a UDP socket on the stack. 
 *
 *  @param address  Destination for the host SocketAddress
 *  @param host     Hostname to resolve
 *  @param version  IP version of address to resolve
 *  @return         0 on success, negative error code on failure
 */
int NetworkInterface::gethostbyname(const char *host, SocketAddress *address, nsapi_version_t version);

/** Add a domain name server to list of servers to query
 *
 *  @param addr     Destination for the host address
 *  @return         0 on success, negative error code on failure
 */
int NetworkInterface::add_dns_server(const SocketAddress &address);

These additions do not require changs to NetworkInterface implementations which rely on the network-socket API's dns_query by default.

cc @sg-, @sarahmarshy, @c1728p9

c1728p9 · 2016-09-08T20:36:37Z

features/net/network-socket/NetworkInterface.h

+     *  @param host     Hostname to resolve
+     *  @return         0 on success, negative error code on failure
+     */
+    virtual int gethostbyname(const char *host, SocketAddress *address);


Is there any reason to make this virtual? In what case would gethostbyname be overridden by an interface?

On second thought probably it probably doesn't hurt to have it match the rest of the files.

It could be useful if a network interface wants to override the gethostbyname implementation for an interface-specific feature.

bogdanm · 2016-09-09T15:25:05Z

features/net/network-socket/nsapi_dns.cpp

+    i = 0;
+
+    for (int count = 0; count < NSAPI_IPv4_BYTES; count++) {
+        int scanned = sscanf(&host[i], "%hhu", &addr->bytes[count]);


Maybe it's a good idea to limit check the number that was scanned?

EDIT: no, sorry, that works fine.

bogdanm · 2016-09-09T15:37:45Z

I would add some new tests (at least for the new IPv4/IPv6 parsing functions).

kjbracey · 2016-09-22T06:41:59Z

I'm not sure why the NetworkStack is being hidden from users. That rather defeats the point of the separation, surely - you're having to add everything to the interface to reveal things that are stack-related, like this.

We still have an utterly fundamental problem in that the resolution is blocking - we do need a parallel non-blocking API for client + Nanostack.

Makes no sense to me that the DNS code has literal parsing. "DNS query" should mean DNS query. It's gethostbyname's job to first attempt local match (as numeric literal, or from table, or constants like "localhost"), then if that fails to call the DNS if present.

If you, say, omitted DNS from the build, you still need gethostbyname to do the non-DNS work. And in the absence of the DNS non-blocking API it's critically important that there are APIs that are guaranteed to not attempt lookup.

Which means retaining the SocketAddress APIs that have literal parsing only. (So why is the parsing code repeated here?).

Basically, I massively favour the approach in #2767.

kjbracey · 2016-09-22T08:13:27Z

@SeppoTakalo

kjbracey · 2016-09-22T11:05:37Z

Another thought - my feeling is that this sort of thing (gethostbyname) being a member function, either of NetworkStack or NetworkInterface, is a design error.

It is not really functionality of either a stack or an interface. It is a utility library call, which may benefit from specification of a stack. It has no need for access to stack internals.

The attempted "hiding" of NetworkStack would be less problematic if resolved by overloads for, say, mbed::gethostbyname(NetworkStack *) and mbed::gethostbyname(NetworkInterface *). You wouldn't have to extend the NetworkInterface class for every single call that really wanted to act on a NetworkStack. You'd just add the necessary overloads to convert Interface to Stack for each non-member functions. (Or maybe define an implicit Interface->Stack conversion operator? Not sure if that's good practice.)

https://stackoverflow.com/questions/5989734/effective-c-item-23-prefer-non-member-non-friend-functions-to-member-functions

But I can see that maybe there's some concern about not defining non-member functions because of the namespace "using mbed" problems (cf mbed::callback).

kjbracey · 2016-09-23T07:33:16Z

Thanks for adopting most of the other approach. Still seems wrong to have the DNS query call contain a literal check. It's just not the conventional way the parts are assembled.

Gethostbyname predates DNS, and was always "number conversion or hosts.txt lookup". The DNS resolver is an extension to that original functionality - a third approach called by gethostbyname.

It logically should be possible to configure out DNS from the build so that "nsapi_dns_query" is a dummy error-returning macro, and leaving the system working. (Or at least it should be structured like that so it's in principle possible).

kjbracey · 2016-09-23T07:38:40Z

features/net/network-socket/nsapi_dns.cpp

+    // check for simple ip addresses
+    SocketAddress address;
+    if (address.set_ip_address(host) && address.get_ip_version() == version) {
+        *addr = address.get_addr();


If it's a valid literal but the version doesn't match, it should return an error rather than proceed to try to resolve it.

Sounds reasonable 👍

geky · 2016-09-23T08:56:08Z

I'm not sure why the NetworkStack is being hidden from users. That rather defeats the point of the separation, surely - you're having to add everything to the interface to reveal things that are stack-related, like this.

Initially the NetworkStack was kept hidden since the full scope of the class and how it interacts with the NetworkInterface was unclear. You're right that with the NetworkStack hidden, the scope of NetworkInterface and NetworkStack starts to merge.

We've been looking into the benefits of both NetworkInterface and NetworkStack class in the user API, but the separation simply doesn't add much and adds to the boilerplate code for setting up networks.

We determined the best path forward would be to combine NetworkStack and NetworkInterface into NetworkInterface, which simply provides a base class for network devices that provide socket-related functions. @c1728p9 was looking into a user-facing NetworkStack class that extends the NetworkInterface and provides stack-related functions (such as registering emacs).

This is currently in work on a separate pr (https://github.com/geky/mbed/tree/nsapi-no-stack, dependent on this and #2664).

geky · 2016-09-23T08:59:41Z

We still have an utterly fundamental problem in that the resolution is blocking - we do need a parallel non-blocking API for client + Nanostack.

This should probably be a separate issue/pr. Does this API limit an asynchronous that could be introduced in the future?

geky · 2016-09-23T09:27:00Z

In regards to how gethostbyname/dns_query is organized

We should provide full gethostbyname behaviour on a UDP socket by default
Network implementations should be able to override the gethostbyname behaviour (ie lwip)

Looking back based on these requirements, I think you're right that there isn't really a need for the literal-parsing behaviour to exist in in dns-query. I'll move it up into the default gethostbyname.

With the merge of NetworkInterface/NetworkStack, is there much reason to have gethostbyname an external function? It is really a behaviour provided by the NetworkThing, not necessarily an operation on the NetworkThing. At this point it boils down to iface->get_ip_address() vs get_ip_address(iface), in which C++ chooses the first.

kjbracey · 2016-09-23T09:50:04Z

I'm doubtful about removing the stack/interface separation. (Unsurprisingly, as I was the one who originally insisted on it).

I'm wary you're looking at "smoothing out" single-interface systems at the expense of potentially making multiple-interface (but single-stack) systems a mess. How many multiple-interface examples do you have? But that's for later discussion.

The blocking thing isn't related to this PR - just highlighting it for awareness. (Would have been relevant on previous PR). The nsapi->stack API is exceptional here in that it is a blocking call to the stack, unlike all the socket calls. However, unlike sockets, I think it's more likely that a stack may only provide blocking resolution :( Will probably need twin APIs to the stack.

The member function thing isn't that big a deal - no terribly strong view on it. See that link earlier for general discussion of the pros and cons - it's not a given that just because it's C++ a member function is to be preferred.

So +2 to this is as it stands now.

geky · 2016-09-23T09:59:46Z

Cool : )

The stack/interface separation should probably be a separate discussion (probably on the pr once dependencies are merged). Does this pr limit our ability to reintroduce the stack/interface separation if need be?

If we find that the gethostbyname would be better structured a different way based on the future NetworkStack class, it seems resonable to move via deprecation. This current model fits well with the single NetworkInterface interface.

kjbracey · 2016-09-23T10:55:12Z

You can always preserve the implicit interface->stack handover to keep it working, regardless of what happens. The interface object must always know its stack object.

sg- · 2016-09-24T13:55:38Z

/morph test

mbed-bot · 2016-09-24T16:11:12Z

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 939

All builds and test passed!

sg- · 2016-09-26T02:55:45Z

@geky needs a rebase and conflicts resolved

For performing DNS resolution explicitly through the network-socket user API. This interface does not require implementation changes and can rely entirely on the dns query library in the network-socket API.

Updated to match prior conventions - netconn_gethostbyname - gethostbyname_r - gethostbyname2_r - gethostbyaddr_r

@mikaleppanen

Merged duplicated logic into the SocketAddress class. Based on parallel work by @mkaleppanen and @kjbracey-arm. Also added small ipv6 parsing fix by @mikaleppanen

- Not inherently a dns operation - Able to reuse SocketAddress provided to the NetworkInterface

geky · 2016-09-26T04:27:12Z

@sg- should be rebased

c1728p9 reviewed Sep 8, 2016
View reviewed changes

bogdanm added needs: work needs: CI labels Sep 9, 2016

bogdanm reviewed Sep 9, 2016
View reviewed changes

sg- removed the needs: CI label Sep 9, 2016

geky mentioned this pull request Sep 21, 2016

Lwip 2.0 #2767

Merged

1 task

geky force-pushed the nsapi-gethostbyname branch from 89fb30e to 8c4ce18 Compare September 21, 2016 21:04

kjbracey reviewed Sep 23, 2016

View reviewed changes

geky force-pushed the nsapi-gethostbyname branch from f5f9ef6 to faf663a Compare September 23, 2016 09:07

geky force-pushed the nsapi-gethostbyname branch from faf663a to d0db597 Compare September 23, 2016 09:34

sg- added needs: CI and removed needs: work labels Sep 24, 2016

sg- added ready for merge and removed needs: CI labels Sep 25, 2016

geky added 5 commits September 25, 2016 22:16

nsapi - Added access to gethostbyname and add_dns_server

3858a13

For performing DNS resolution explicitly through the network-socket user API. This interface does not require implementation changes and can rely entirely on the dns query library in the network-socket API.

nsapi - Reversed arguments to gethostbyname

d237ee8

Updated to match prior conventions - netconn_gethostbyname - gethostbyname_r - gethostbyname2_r - gethostbyaddr_r

nsapi - Folded IP parsing logic into SocketAddress

370afe5

Merged duplicated logic into the SocketAddress class. Based on parallel work by @mkaleppanen and @kjbracey-arm. Also added small ipv6 parsing fix by @mikaleppanen

nsapi - Increased DNS buffer size to 512 bytes per spec

2194ca1

nsapi - Moved literal ip parsing out of dns-query

4f7b10f

- Not inherently a dns operation - Able to reuse SocketAddress provided to the NetworkInterface

geky force-pushed the nsapi-gethostbyname branch 2 times, most recently from 29aa023 to 4f7b10f Compare September 26, 2016 04:26

sg- merged commit f93f484 into ARMmbed:master Sep 26, 2016

sg- added release-version: 5.2.0 and removed ready for merge labels Sep 26, 2016

nsapi - Add explicit DNS interface to the network socket API #2652

nsapi - Add explicit DNS interface to the network socket API #2652

Uh oh!

Conversation

geky commented Sep 8, 2016

Uh oh!

c1728p9 Sep 8, 2016

Choose a reason for hiding this comment

Uh oh!

c1728p9 Sep 8, 2016

Choose a reason for hiding this comment

Uh oh!

geky Sep 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bogdanm Sep 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bogdanm commented Sep 9, 2016

Uh oh!

kjbracey commented Sep 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kjbracey commented Sep 22, 2016

Uh oh!

kjbracey commented Sep 22, 2016

Uh oh!

kjbracey commented Sep 23, 2016

Uh oh!

kjbracey Sep 23, 2016

Choose a reason for hiding this comment

Uh oh!

geky Sep 23, 2016

Choose a reason for hiding this comment

Uh oh!

geky commented Sep 23, 2016

Uh oh!

geky commented Sep 23, 2016

Uh oh!

geky commented Sep 23, 2016

Uh oh!

kjbracey commented Sep 23, 2016

Uh oh!

geky commented Sep 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kjbracey commented Sep 23, 2016

Uh oh!

sg- commented Sep 24, 2016

Uh oh!

mbed-bot commented Sep 24, 2016

Result: SUCCESS

Output

Uh oh!

sg- commented Sep 26, 2016

Uh oh!

geky commented Sep 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

geky Sep 8, 2016 •

edited

Loading

bogdanm Sep 9, 2016 •

edited

Loading

kjbracey commented Sep 22, 2016 •

edited

Loading

geky commented Sep 23, 2016 •

edited

Loading

geky commented Sep 26, 2016 •

edited

Loading