Add non-blocking DNS functionality to network interface #6751

mikaleppanen · 2018-04-26T09:17:21Z

Description

Added non-blocking DNS interface to network interface and network stack.
Added caching of DNS replies.
Added a network stack function to get DNS addresses from stack.
Added call and call_in hooks to onboard network stack to allow calling functions from onboard stack context.
Added support to call and call_in functions to LWIP and Nanostack.
Disabled LWIP DNS translator with the exception of DNS address storage used in DNS address get.

Pull request type

[ ] Fix
[ ] Refactor
[ ] New target
[X] Feature
[ ] Breaking change

mikaleppanen · 2018-04-26T09:18:03Z

@kjbracey-arm @SeppoTakalo Please review

0xc0170 · 2018-04-26T12:58:27Z

Please review jenkins CI (undefined functions that are being added here?)

kjbracey · 2018-04-26T11:50:15Z

features/FEATURE_LWIP/lwip-interface/lwip-sys/arch/lwip_sys_arch.c

+
+bool sys_tcpip_thread_check(void)
+{
+    osThreadId_t thread_id = osThreadGetId();


I'm very anti-verbosity, so I would have written this as one line:

return osThreadGetId() == lwip_tcpip_thread_id;

Maybe not to your taste.

kjbracey · 2018-04-26T11:51:14Z

features/FEATURE_LWIP/lwip-interface/lwip-sys/arch/lwip_sys_arch.c

@@ -484,6 +484,24 @@ void sys_msleep(u32_t ms) {
    osDelay(ms);
 }

+osThreadId_t lwip_tcpip_thread_id = 0;
+
+bool sys_tcpip_thread_set(void)


void return

kjbracey · 2018-04-26T12:16:04Z

features/netsocket/nsapi_dns.cpp

+    return NSAPI_ERROR_OK ;
+}
+
+static nsapi_error_t nsapi_dns_call_in_default(int delay, mbed::Callback<void()> func)


Suggestion - these become the base implementation of NetworkStack::call, and then the logic below becomes if (dns_call) { custom} else { stack->call }.

kjbracey · 2018-04-26T12:17:27Z

features/netsocket/OnboardNetworkStack.h

+    /** Call a callback
+     *
+     *  Call a callback from the network stack context. If returns error
+     *  callback will not be called.


We need a bit of text about what you can/can't do from here. Don't take significant time, and specify that of the network calls, only non-blocking UDP/datagram calls can be used.

This is a strange API for a network stack.

Why not expose an event queue by pointer and return NULL if there isn't one used.

Then the user could choose a timer or eventqueue based on their needs.

But also +1 for mentioning that long functions could mess with the network stack.

Actually thinking about it more this just seems like the wrong direction?

See below

kjbracey · 2018-04-26T12:18:46Z

features/netsocket/nsapi_dns.cpp

+
+    // Allocates in case entry is free, otherwise reuses
+    if (!dns_cache[index]) {
+        dns_cache[index] = new DNS_CACHE;


This will explode on allocation failure - probably want new (nothrow) DNS_CACHE here with a null check.

kjbracey · 2018-04-26T12:26:58Z

features/netsocket/nsapi_dns.h

+ *                  NSAPI_ERROR_DNS_FAILURE indicates the host could not be found
+ */
+nsapi_error_t nsapi_dns_query_async(NetworkStack *stack, const char *host,
+        NetworkStack::hostbyname_cb_t callback, void *data, nsapi_version_t version = NSAPI_IPv4);


This should be a Callback - that's slightly more flexible.

See https://os.mbed.com/docs/v5.7/reference/platform.html#callbacks

Keep in mind this layer also supports C. But it would be useful to have a C++ overload. The callback class has a thunk member function that can be used when passing to C layer:

void hi(void (*cb)(void *data, int arg), void *data); Callback<void(int)> cb = function_here; hi(&Callback<void(int)>::thunk, &cb);

kjbracey · 2018-04-26T12:28:02Z

features/netsocket/nsapi_dns.cpp

+            return;
+        }
+
+        nsapi_addr_t *addrs = new nsapi_addr_t[query->addr_count];


Mixing C/C++ allocator types is a bit odd. Not going to rule it out, but these C++ ones should probably be nothrow with null checks.

kjbracey · 2018-04-26T12:29:38Z

features/netsocket/nsapi_dns.cpp

+        nsapi_addr_t *addrs = new nsapi_addr_t[query->addr_count];
+
+        uint32_t ttl;
+        int count = dns_scan_response((const uint8_t *) packet, id, &ttl, addrs, query->addr_count);


Unnecessary cast?

kjbracey · 2018-04-26T12:29:47Z

features/netsocket/nsapi_dns.cpp

+        }
+
+        // gets id from response to associate with correct query
+        uint16_t id = (*packet << 8) | *(packet + 1);


style nit - I think (packet[0] << 8) | packet[1] is nicer.

SeppoTakalo · 2018-04-26T13:42:54Z

features/netsocket/NetworkInterface.h

+     *  @return         0 on success, negative error code on failure
+     */
+    virtual nsapi_error_t gethostbyname_async(const char *host, hostbyname_cb_t callback, void *data,
+            nsapi_version_t version = NSAPI_UNSPEC);


As discussed, drop the data as callback object can already encapsulate it for you.

Other than that +1 for the API.

SeppoTakalo · 2018-04-26T13:46:10Z

features/netsocket/NetworkStack.cpp

+            return NSAPI_ERROR_DNS_FAILURE;
+        }
+
+        return NSAPI_ERROR_OK;


Did I miss something?
SocketAddress address is local object.
And you return with NSAPI_ERROR_OK but where do you actually call the callback with this new address, if it was in fact numeric and conversion was OK.

SeppoTakalo · 2018-04-26T13:59:31Z

features/netsocket/NetworkStack.h

+     *  @return         0 on success, negative error code on failure
+     */
+    virtual nsapi_error_t gethostbyname_async(const char *host, hostbyname_cb_t callback, void *data,
+            nsapi_version_t version = NSAPI_UNSPEC);


API is identical with NetworkInterface and NetworkStack so can you move these both to one pure virtual class that both can inherit.

If/when the callback can be called synchronously during the gethostbyname_async(), could it be documented here too? Just to avoid surprises.

I'll add note about that.

kjbracey · 2018-04-27T07:22:56Z

I'd also like @geky and @c1728p9 to cast an eye over this.

geky · 2018-04-27T10:59:55Z

@kjbracey-arm, be careful what you ask for : )

kjbracey · 2018-04-27T11:02:49Z

I'm confident @mikaleppanen's work can withstand the scrutiny. Pretty happy that this is good, just want to make sure it's perfect. Pretty important bit of API.

geky · 2018-04-27T11:03:10Z

features/netsocket/NetworkInterface.h

+     *  @param address On success, destination for the host SocketAddress
+     *  @param data    Caller defined data
+     */
+    typedef mbed::Callback<void (nsapi_error_t result, SocketAddress *address, void *data)> hostbyname_cb_t;


I think callback already handles data for you?

geky · 2018-04-27T11:05:25Z

features/netsocket/NetworkStack.cpp

 nsapi_error_t NetworkStack::add_dns_server(const SocketAddress &address)
 {
    return nsapi_dns_add_server(address);
 }

+nsapi_error_t NetworkStack::get_dns_server(int index, SocketAddress *address)
+{
+    return NSAPI_ERROR_UNSUPPORTED;


Couldn't the default list of dns servers go here?

This is only to return stack specific addresses from DHCP/PPP for network stacks that support those (only LWIP at the moment).

geky · 2018-04-27T11:09:48Z

features/netsocket/NetworkStack.h

     *
     *  @return         Null-terminated representation of the local IP address
     *                  or null if not yet connected
     */
-    MBED_DEPRECATED_SINCE("mbed-os-5.7",
-        "Use NetworkInterface::get_ip_address()")
    virtual const char *get_ip_address();


Why is this being undeprecated?

It actually never has been - that's a proposed deprecation on the feature-emac branch.

On reflection we've decided this is too useful as "get primary network address for this stack", which is what the DNS code among other wants to do to get hints for IPv6-ness, so we've decided not to deprecate.

Note that DNS is definitely a stack thing, but there's an issue that we've no way to get from stack to interface(s). If we were to add NetworkInterface *NetworkStack::get_default_interface() or NetworkInterface *NetworkStack::get_interface(int), we would be able to use those, rather than have these funny network stack calls that implicitly map to default interface.

Ah I see 👍

geky · 2018-04-27T11:13:02Z

features/netsocket/OnboardNetworkStack.h

+    /** Call a callback
+     *
+     *  Call a callback from the network stack context. If returns error
+     *  callback will not be called.


This is a strange API for a network stack.

Why not expose an event queue by pointer and return NULL if there isn't one used.

Then the user could choose a timer or eventqueue based on their needs.

But also +1 for mentioning that long functions could mess with the network stack.

geky · 2018-04-27T11:17:35Z

features/netsocket/nsapi_dns.h

+ *                  NSAPI_ERROR_DNS_FAILURE indicates the host could not be found
+ */
+nsapi_error_t nsapi_dns_query_async(NetworkStack *stack, const char *host,
+        NetworkStack::hostbyname_cb_t callback, void *data, nsapi_version_t version = NSAPI_IPv4);


Keep in mind this layer also supports C. But it would be useful to have a C++ overload. The callback class has a thunk member function that can be used when passing to C layer:

void hi(void (*cb)(void *data, int arg), void *data); Callback<void(int)> cb = function_here; hi(&Callback<void(int)>::thunk, &cb);

geky · 2018-04-27T11:18:54Z

features/netsocket/nsapi_dns.h

+ *                    NSAPI_ERROR_DNS_FAILURE indicates the host could not be found
+ */
+nsapi_size_or_error_t nsapi_dns_query_multiple_async(NetworkStack *stack, const char *host,
+        NetworkStack::hostbyname_cb_t callback, void *data, nsapi_size_t addr_count, nsapi_version_t version = NSAPI_IPv4);


Should there be a C verions of nsapi_dns_query_multiple_async? If so it'd need the extern "C" and stuff.

geky · 2018-04-27T11:25:34Z

features/netsocket/nsapi_dns.cpp

+{
+    if (stack->onboardNetworkStack()) {
+        OnboardNetworkStack *onboard_stack = reinterpret_cast<OnboardNetworkStack *>(stack);
+        return onboard_stack->call(func);


Thinking about it more, I think this is just a bit too upside down. We're poking all these APIs through the network stack because we need them from an API that's called internally?

Why not just pass the necessary things to the more-or-less internal API?

If the dns_query_async API needs an event loop, just make that a necessary argument to the function?

The logic here is that we've hit a repeating requirement for network-related stuff to do little tasks. Things like PHY status monitoring in Ethernet drivers. Previously lwip/Nanostack drivers would use lwip/Nanostack event APIs to schedule those. In the EMAC world that's out, and they've been tending to switch to mbed global EventQueues.

Which isn't bad, but it is possible they end up being the only user of the global event queue. It will be the case in the smallest mini client.

So the thought is that we know any onboard network stack must have some sort of event-like mechanism, so this genericises the "run in stack context" drivers have historically used, and it remains the case that it will never spawn an extra thread. We were thinking of Ethernet drivers as much as DNS.

The need for this special API would be eliminated if we required OnboardNetworkStacks to be using EventQueue for their processing, but that's not the case for either LWIP or Nanostack at the moment.

But these all seem like things that are called from the network stack, so if the network stack has an event queue couldn't it just be passed to the ethernet drivers at call/init time?

Yes, it effectively would be for drivers - we're not doing that yet. But it's not necessarily an mbed OS EventQueue.

For DNS API purposes the callback context for DNS completion can be API-user specified if they have special requirements for their own event system, or they can say "don't care", in which case they end up getting called in the network stack's context, or the global EventQueue which acts as the default for NetworkStack.

(The whole DNS state machine runs in a single context, so that user-specified context setting has to be global, not per query. They can always use a trampoline to bounce per call if necessary).

Certainly simplest thing for DNS async API would be to require an EventQueue to run on, and default to the global one. But that would mean an extra thread with LWIP and Nanostack, which is what we're trying to avoid. And we're not facing up to restructuring their respective event/thread model to use EventQueue.

In that case, do you think we need an abstract EventQueue interface that could be backed by different event queue implementation? Then the DNS/EMAC apis could take in an "EventQueueHandle" that could be backed by the native nanostack event queue.

That may also be a good place to put all of the template madness for the event queue api.

(Name is a joke)

Yes, I was thinking something like that might be in order, and was pondering suggesting it. Maybe if we start unambitiously with just the call/call_in API and we can flesh it out later.

(Only reason I was hesitating suggesting it was possible delivery time impact for this PR)

Yeah, it would probably take a while. Also not sure it's entirely possible to combine the virutal class with the templating inside the EventQueue.

Maybe you could just pass a pointer to the call_in function?

void emac_function(Callback<int(int ms, Callback<void()> cb)> call_in_cb);

(you could also use call_in(0, blah) for the call function without a timeout if you want one less argument)

I'll change the API so that network stack provides private method to get the callback and that is passed to DNS resolver. That way there will be no public call/call_in interface. Also EMAC can in future can use same kind of interface (as described above).

geky · 2018-04-27T11:25:53Z

features/netsocket/OnboardNetworkStack.h

+    /** Call a callback
+     *
+     *  Call a callback from the network stack context. If returns error
+     *  callback will not be called.


Actually thinking about it more this just seems like the wrong direction?

See below

geky · 2018-04-27T11:26:40Z

features/netsocket/OnboardNetworkStack.h

@@ -39,6 +39,8 @@ class OnboardNetworkStack : public NetworkStack {
     */
    static OnboardNetworkStack &get_default_instance();

+    virtual OnboardNetworkStack *onboardNetworkStack() { return this; }


See below (or above, depending on GitHub ordering). I don't think we need this on this pr

geky · 2018-04-27T11:30:43Z

I left a few comments, but honestly I can't keep up with fully review these prs. So don't let me block anything, and feel free to do whatever you want as long as you take responsibility for the code >: )

c1728p9 · 2018-04-28T18:12:02Z

features/netsocket/nsapi_dns.cpp

+    }
+
+    // Checks if already cached
+    if (nsapi_dns_cache_find(host, address->get_ip_version(), NULL) == NSAPI_ERROR_OK) {


Should this be inside the lock so the state reflects the return value of nsapi_dns_cache_find?

c1728p9 · 2018-04-28T18:12:27Z

features/netsocket/nsapi_dns.cpp

+    }
+
+    if (index < 0) {
+        return;


You need an unlock here.

c1728p9 · 2018-04-28T18:19:46Z

features/netsocket/nsapi_dns.h

+ *  @return         0 on success, negative error code on failure
+ *                  NSAPI_ERROR_DNS_FAILURE indicates the host could not be found
+ */
+nsapi_error_t nsapi_dns_query_async(NetworkStack *stack, const char *host,


There should probably be an abort function to go along with each of these async starts. That way objects using the async APIs can properly cleanup in their destructor if they are destroyed before the async operation has been completed.

I agree. The cancel needs to be added to this API.

I'll add cancel to the interface.

Thanks, with a cancel this PR is a dream come true to us.

marcuschangarm · 2018-04-30T19:12:46Z

features/netsocket/nsapi_dns.cpp

            continue;
        }

        // recv the response
        err = socket.recvfrom(NULL, packet, DNS_BUFFER_SIZE);
        if (err == NSAPI_ERROR_WOULD_BLOCK) {
+            if (!retry) {
+                // retries once


I've been experimenting with wifi on a bad connection and I often need to do multiple DNS lookups for it to go through. Would it be possible to make the retry number configurable and not fixed to one, please?

I'll add json configuration options about retries.

TeroJaasko · 2018-05-03T09:31:31Z

features/netsocket/NetworkStack.h

+     *  @param address On success, destination for the host SocketAddress
+     *  @param data    Caller defined data
+     */
+    typedef mbed::Callback<void (nsapi_error_t result, SocketAddress *address, void *data)> hostbyname_cb_t;


As @kjbracey-arm mentioned elsewhere, the documentation of callback should state what can be done from it. I guess typical application would want to connect a socket on success case, or issue another DNS query on failure.

I'll add a note about limitations. Since this is always running from thread it is possible to do more than e.g. for socket sigio(). But thread stack size limits what can be initiated.

If stack sizes are configured larger (lwip, nanostack or global event queue thread) or if application provides callback using nsapi_dns_call_in_set(), at least non-blocking UDP operations and asynchronous DNS query are possible from callback.

TeroJaasko · 2018-05-03T09:57:23Z

features/netsocket/NetworkStack.h

+     *  @param address On success, destination for the host SocketAddress
+     *  @param data    Caller defined data
+     */
+    typedef mbed::Callback<void (nsapi_error_t result, SocketAddress *address, void *data)> hostbyname_cb_t;


If the query returns multiple addresses, is there a way to iterate through all of them, just as with getaddrinfo() on POSIX?

Like previous blocking interface, multiple addresses are not supported. That might be done later as a new extension to interface.

TeroJaasko · 2018-05-03T10:02:20Z

features/netsocket/nsapi_dns.cpp

+
+struct DNS_CACHE {
+    SocketAddress address;
+    char host[128];


If RFC 1035 states that full DNS name can be 255 bytes, could this non-standard limitation be specified by some configuration option. I have no objection for limiting this by default, but if the max length was exposed via a public define, the client application could use it for its own buffers too. And a space limited application might try to tune the max name length and cache size.

I'll add configuration options to json for both name length and number of entries in cache.

We have hit problems with very long Amazon server names - not sure what limit was being breached there. Maybe 64 characters in the ESP8266?

I'd like the default to be the standard if it possibly fits. For a cache with your own internal structures, maybe claw some back by using the nsapi_addr_t, skipping the string literal representation?

Changed limit to 255 and changed storage to use dynamic memory for host name.

TeroJaasko · 2018-05-03T13:06:06Z

features/nanostack/FEATURE_NANOSTACK/nanostack-interface/Nanostack.cpp

+        }
+    } else {
+        if (eventOS_event_send(&event) < 0) {
+            return NSAPI_ERROR_NO_MEMORY;


Shouldn't the cb be deleted on these error cases to avoid memory leaks?

mikaleppanen · 2018-05-04T06:34:41Z

Pushed a commit that contains fixes for review defects. Please review the corrections.

kjbracey · 2018-05-04T10:39:11Z

features/netsocket/NetworkStack.h

+     *  @param callback Callback that is called for result
+     *  @param version  IP version of address to resolve, NSAPI_UNSPEC indicates
+     *                  version is chosen by the stack (defaults to NSAPI_UNSPEC)
+     *  @return         0 on success, negative error code on failure or an unique id that


To pin it down a little: "immediate success"? "immediate failure"? "positive unique id"? Kind of obvious, but just to be utterly clear.

And given it's a 3-way split, maybe put it as 3 separate @return lines rather than 1 long run-on.

kjbracey · 2018-05-04T10:39:40Z

features/netsocket/NetworkStack.h

+     *                  represents the hostname translation operation and can be passed to
+     *                  cancel
+     */
+    virtual nsapi_error_t gethostbyname_async(const char *host, hostbyname_cb_t callback,


We have nsapi_size_or_error_t. Wondering if we should have something like that here rather than just using nsapi_error_t. Or maybe make it return int. Passing nsapi_error_t to cancel is weird - I'd like that to take int or some sort of nsapi_id_t. (int is probably fine)

Added new definition: nsapi_value_or_error_t

kjbracey · 2018-05-04T10:42:52Z

features/netsocket/nsapi_dns.cpp

 #define DNS_SERVERS_SIZE 5
+#define DNS_RESPONSE_MIN_SIZE 12
+#define DNS_MAX_TTL 604800


Would help to have this as an expression and comment to clarify the derivation. I gather it's (7*24*60*60) \\ 7 days in seconds.

Although, is there actually any requirement to have this clamp at the client end? Reference?

Removed that.

kjbracey · 2018-05-04T10:53:11Z

features/netsocket/nsapi_dns.cpp

+
+    if (dns_cache[index]) {
+        dns_cache[index]->address = *address;
+        dns_cache[index]->host = (char *) malloc(strlen(host) + 1);


If we're being C++, new (nothrow) char[strlen(host) + 1]?

Changed to new

kjbracey · 2018-05-04T10:54:45Z

features/netsocket/nsapi_dns.cpp

+        dns_cache[index]->host = (char *) malloc(strlen(host) + 1);
+        strcpy(dns_cache[index]->host, host);
+        uint64_t ms_count = rtos::Kernel::get_ms_count();
+        dns_cache[index]->expires = ms_count + ttl * 1000;


Technically (if you weren't clamping ttl), you'd want to do something to make sure you don't overflow a 32-bit multiply - (uint64_t) ttt or UINT64_C(1000). This is probably utterly pointless, of course.

Added casting.

kjbracey · 2018-05-04T11:05:51Z

features/netsocket/mbed_lib.json

+            "value": 3
+        },
+        "dns-retries": {
+            "help": "Number of DNS query retries that the DNS translator makes per server",


Whichever you decide for the previous, seems like it might be best to have the two settings phrased the same. I think this needs a bit more explanation - before moving on to the next server. Total retries/attempts is always limited by "dns-total-retries/attempts".

Updated the description.

kjbracey · 2018-05-04T11:10:49Z

features/netsocket/nsapi_dns.cpp

+static void nsapi_dns_cache_add(const char *host, nsapi_addr_t *address, uint32_t ttl)
+{
+    // RFC 1034: if TTL is zero, entry is not added to cache
+    if (!ttl) {


As this is a number, not a boolean, I prefer if (ttl == 0)

kjbracey · 2018-05-04T11:11:45Z

features/netsocket/nsapi_dns.cpp

+    dns_cache_mutex.lock();
+
+    int index = -1;
+    uint64_t accessed = ~0;


Maybe UINT64_MAX? Don't feel strongly.

kjbracey · 2018-05-04T11:14:50Z

features/netsocket/nsapi_dns.cpp

+#define DNS_HOST_NAME_MAX_LEN 255
+
+#ifndef MBED_CONF_NSAPI_DNS_RESPONSE_WAIT_TIME
+#define MBED_CONF_NSAPI_DNS_RESPONSE_WAIT_TIME   5000


Do we need these defaults? I just helped someone out who was confusing himself by trying to change lines like these, rather than the JSON file, to no effect.

Removed those.

kjbracey · 2018-05-04T11:18:06Z

features/netsocket/nsapi_dns.cpp

+                query->addrs = 0;
+            } else {
+                query->count = resp;
+                nsapi_dns_call_in(query->call_in_cb, 0, mbed::callback(nsapi_dns_query_async_response, reinterpret_cast<void *>(query->unique_id)));


Is this cast necessary?

Yes, seems that adding an int to callback is not possible.

- Added non-blocking DNS interface to network interface and network stack. - Added caching of DNS replies. - Added a network stack function to get DNS addresses from stack. - Added call and call_in hooks to onboard network stack to allow calling functions from onboard stack context. - Added support to call and call_in functions to LWIP and Nanostack. - Disabled LWIP DNS translator with the exception of DNS address storage used in DNS address get.

- Changed call_in/call methods of the stack to callback provided by the stack - Specified what are limitations for operations that are made in callback - Added pure virtual class DNS that defines DNS operations - Added cancel operation and unique ID to DNS request used in cancel - Added DNS configuration options to netsocket/mbed_lib.json for retries, response wait time and cache size - Changed host name to use dynamic memory in DNS query list and cache, set maximum length for the name to 255 bytes. - Added mutex to asynchronous DNS - Reworked retries: there is now total retry count and a server specific count - Ignores invalid incoming UDP socket messages (DNS header is not valid), and retries DNS query - Reworked DNS module asynchronous operation functions - Corrected other review issues (nothrow new, missing free, missing mutex unlock etc.)

- Serialized the sending of multiple async DNS queries since limits on event message box sizes - Added timer function that supervises the total time used to make DNS query and triggers socket timeouts - Changed nsapi_error_t to new nsapi_value_or_error_t on interface headers - Corrected wording of gethostbyname_async return values - Clarified .json options - Added a new data type for socket callback that can be used from interrupts - Corrected variable limits to use INT32_MAX etc. defines - Changed mallocs to new - Optimized variable sizes on DNS_QUERY definition

mikaleppanen · 2018-05-14T10:31:20Z

Corrected defects, please review.

cmonr · 2018-05-14T16:20:42Z

/morph build

mbed-ci · 2018-05-14T17:48:06Z

Build : SUCCESS

Build number : 2009
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/6751/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

mbed-ci · 2018-05-15T07:13:16Z

Test : FAILURE

Build number : 1819
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/6751/1819

TeroJaasko · 2018-05-15T10:15:58Z

features/netsocket/nsapi_dns.cpp

+// Protects cache shared between blocking and asynchronous calls
+static rtos::Mutex dns_cache_mutex;
+// Protects from several threads running asynchronous DNS
+static rtos::Mutex dns_mutex;


Should these be PlatformMutex:es instead to support non-RTOS compilation?

Yes, they can be. Changed them into PlatformMutex:es.

TeroJaasko · 2018-05-15T10:20:29Z

features/netsocket/nsapi_dns.cpp

+
+        nsapi_dns_query_async_resp(query, status, addresses);
+
+        if (addresses) {


No need to check for NULL before calling operator delete.

TeroJaasko

+3

- Changed mutexes to platform mutexes - Removed not needed null check from delete - Changed nsapi_dns_call_in_set() to use call_in_callback_cb_t and added prototype to header

mbed-ci · 2018-05-15T13:38:28Z

Exporter Build : FAILURE

Build number : 1661
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/6751/

cmonr · 2018-05-15T14:46:09Z

/morph export-build

cmonr · 2018-05-18T03:16:50Z

@kjbracey-arm Echoing an earlier question. Why are you assigning release-version tags to PRs going into the feature-emac branch?

@adbridge

0xc0170 requested review from kjbracey and SeppoTakalo April 26, 2018 09:40

0xc0170 added the needs: review label Apr 26, 2018

0xc0170 changed the title ~~Added non-blocking DNS functionality to network interface~~ Add non-blocking DNS functionality to network interface Apr 26, 2018

kjbracey reviewed Apr 26, 2018

View reviewed changes

SeppoTakalo suggested changes Apr 26, 2018

View reviewed changes

0xc0170 added needs: work and removed needs: review labels Apr 26, 2018

0xc0170 requested review from geky and c1728p9 April 27, 2018 09:06

geky reviewed Apr 27, 2018

View reviewed changes

c1728p9 reviewed Apr 28, 2018

View reviewed changes

marcuschangarm reviewed Apr 30, 2018

View reviewed changes

TeroJaasko reviewed May 3, 2018

View reviewed changes

kjbracey reviewed May 4, 2018

View reviewed changes

kjbracey force-pushed the feature-emac branch from ea912d5 to 942209e Compare May 9, 2018 07:09

Mika Leppänen added 3 commits May 14, 2018 11:12

kjbracey approved these changes May 14, 2018

View reviewed changes

SeppoTakalo approved these changes May 14, 2018

View reviewed changes

cmonr added needs: CI and removed needs: work labels May 14, 2018

TeroJaasko reviewed May 15, 2018

View reviewed changes

TeroJaasko approved these changes May 15, 2018

View reviewed changes

Changed mutexes, delete and DNS call in callback set

21d21c5

- Changed mutexes to platform mutexes - Removed not needed null check from delete - Changed nsapi_dns_call_in_set() to use call_in_callback_cb_t and added prototype to header

kjbracey mentioned this pull request May 15, 2018

Feature-emac combined merge #6903

Merged

kjbracey added do not merge and removed needs: CI labels May 15, 2018

kjbracey merged commit 21d21c5 into ARMmbed:feature-emac May 17, 2018

kjbracey added release-version: 5.9.0-rc1 and removed do not merge labels May 17, 2018

kjbracey removed the release-version: 5.9.0-rc1 label May 18, 2018

This was referenced May 21, 2018

Updated ODIN drivers to v2.5.0 RC1 #6913

Closed

Merge feature-emac branch into master #6847

Merged


		nsapi_dns_query_async_resp(query, status, addresses);

		if (addresses) {

Add non-blocking DNS functionality to network interface #6751

Add non-blocking DNS functionality to network interface #6751

Uh oh!

Conversation

mikaleppanen commented Apr 26, 2018

Description

Pull request type

Uh oh!

mikaleppanen commented Apr 26, 2018

Uh oh!

0xc0170 commented Apr 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kjbracey commented Apr 27, 2018

Uh oh!

geky commented Apr 27, 2018

Uh oh!

kjbracey commented Apr 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geky Apr 27, 2018 •

edited

Loading

geky Apr 27, 2018 •

edited

Loading