Add Getaddrinfo interface for multiple DNS adresses

[tymoteuszblochmobica]

Description

Added getaddrinfo interface for multiple DNS adresses.
It uses existing nsapi_dns_query_multiple functions.
Sync and async version added to interface so it requires dependent libraries rebuild.

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[x] Breaking change

Reviewers

@mikaleppanen
@SeppoTakalo
@kjbracey-arm
@kivaisan
@AriParkkila

Release Notes

New members are added to the network interface
-getaddrinfo
-getaddrinfo_async
Both of them allocates space for results.
Function gethostbyname is unchanged but gethostbyname_async/getaddrinfo_async callback result param now contains number of DNS records found instead NSAPI_ERROR_OK =0 so definition of new callback for getaddrinfo_async is not needed.

Test cases for sync/async added added to DNS test folder.

SYNCHRONOUS_DNS_MULTI_IP
ASYNCHRONOUS_DNS_MULTI_IP

[ciarmcom]

@tymoteuszblochmobica, thank you for your changes.
@kivaisan @mikaleppanen @kjbracey-arm @AriParkkila @SeppoTakalo @ARMmbed/mbed-os-ipcore @ARMmbed/mbed-os-maintainers please review.

[SeppoTakalo]

Why diversion from POSIX API?
See Getaddrinfo: https://pubs.opengroup.org/onlinepubs/9699919799/functions/freeaddrinfo.html

This is basically equivalent, return list of addresses to go through. I would prefer the POSIX, instead of making a new ways to achieve something that there has been standards for.

[tymoteuszblochmobica]

Ok i can replace gethostbyname_multiple with

int getaddrinfo(const char *restrict nodename,
const char *restrict servname,
const struct addrinfo *restrict hints,
struct addrinfo **restrict res);

However need parameter for maximum adress count.
Its needed by :
nsapi_dns_query_multiple(NetworkStack *stack, const char *host,
SocketAddress *addr, nsapi_size_t addr_count, const char *interface_name, nsapi_version_t version = NSAPI_IPv4);
How can i get it?

• add new config like MBED_CONF_NSAPI_DNS_MAX_ADRESSES ?
• use one of unused existing or add new field in addrinfo *restrict hint?

struct addrinfo {
int ai_flags; maybe use this?
int ai_family;
int ai_socktype;
int ai_protocol;
socklen_t ai_addrlen;
struct sockaddr *ai_addr;
char *ai_canonname;
struct addrinfo *ai_next;
};

[SeppoTakalo]

Is it possible to use SocketAddres instead of struct addrinfo, as many of the fields already exist there? Also, I'm not sure we can use service name, we don't have registry for that.

So proposal would be:

/**
 * Translate DNS hostname to list of addresses that match given hints.
 * @param hostname DNS address to query.
 * @param hints Port and address version to match.
 * @param **res Pointer where the resulting pointer will be written on success.
 * @return number of responses on success.
 * @return negative error code on failure
 */
int getaddrinfo(const char *hostname, SocketAddress hints, SocketAddress **res);

Usage example could be:

#define HTTP_PORT 80
SocketAddress hints{{NSAPI_IPv4}, HTTP_PORT};
SocketAddress *result;

int nres = getaddrinfo("google.com", hints, &result);
for (int i = 0; i < nres; i++) {
    TCPSocket s;
    s.open(interface);
    s.connect(result[i]);
}

Alternatively you could give hints like: SocketAddress hints{{NSAPI_UNSPEC}, 443};

It is not exactly POSIX, but something that you can easily relate to. Also does not need any extra types to be defined. Currently, I don't see any usecase for those fields in struct addrinfo which we don't currently have in Mbed OS.

[tymoteuszblochmobica]

Its ok but im going to reuse nsapi_dns_query_multiple and this function needs maximum address count as one of arguments.
There are two options
define new constant like MBED_CONF_NSAPI_DNS_MAX_DNS_ADDR
or pass it from getaddrinfo
i prefer to add MBED_CONF_NSAPI_DNS_MAX_DNS_ADDR
and user can override it in app json.
Also interface_name is needed to get proper DNS server. So it cannot be 100% POSIX compatible.

[tymoteuszblochmobica]

According to POSIX, async version needs to be added either.
int getaddrinfo_a(int mode, struct gaicb *list[],
int nitems, struct sigevent *sevp)

struct gaicb {
const char *ar_name;
const char *ar_service;
const struct addrinfo *ar_request;
struct addrinfo *ar_result;
};
Will replace addrinfo with SocketAddress but need to define gaicb struct.
Im wondering where to emplace interface_name parameter.

[tymoteuszblochmobica]

Origin definition
int getaddrinfo(const char* hostname,
const char* service,
const struct addrinfo* hints,
struct addrinfo** res);
Changed to:
nsapi_error_t getaddrinfo(const char hostname, const char interface_name, SocketAddress *hints, SocketAddress **res ) = 0;

We don use const char* service, but need interface name so it is replaced with interface_name to support multihoming DNS server select.

Is it ok?

[SeppoTakalo]

Can we move the interface_name to last parameter and allow it to be optional?

Most users don't specify the interface.

[tymoteuszblochmobica]

Yes we can, but argument order will not be the same as original getaddrinfo

[kivaisan]

temp can now be NULL in case of out-of-memory, case, so should there be check for its validity? nsapi_dns_query_multiple does not seem to check it either.

[tymoteuszblochmobica]

Done

[kivaisan]

Also update PR description gethostbyname -> getaddrinfo

[teetak01]

@tymoteuszblochmobica still requires 4.2.0 release. The change was only internal.

[tymoteuszblochmobica]

I mean testing purpose if whole CI pass successfuly after restart.

[michalpasztamobica]

@0xc0170 , just out of curiosity. If the CI has now turned green, it means that it checked out the latest master of mbed-client-pal (not the released one)? I tried to check internal CI scripts myself but it's black magic to me...
If I got it right then now we are only waiting for the release in order not to have mbed-cloud-client broken and we are double sure, that the new API works fine for mbed-cloud-client.

[tymoteuszblochmobica]

CI can be restarted

[mbed-ci]

Test run: FAILED

Summary: 1 of 11 test jobs failed
Build number : 7
Build artifacts

Failed test jobs:

• jenkins-ci/mbed-os-ci_greentea-test

[michalpasztamobica]

The issue seems to be caused by some RAAS glitch:

[1576757915.25][urllib3.connectionpool]Starting new HTTPS connection (1): mervi.mbedcloudtesting.com:443
[1576757918.56][urllib3.connectionpool]https://mervi.mbedcloudtesting.com:443 "PUT /resource/07640221184061162704F74F/release HTTP/1.1" 200 62
[1576757918.56][HTST][ERR] remote write error: Write response(request_id: a0144274-2259-11ea-a2a6-0242ac110005) timeout!
[1576757918.56][HTST][WRN] stopped to consume events due to __notify_conn_lost event

[adbridge]

CI restarted

[mbed-ci]

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 8
Build artifacts

[0xc0170]

all ready @tymoteuszblochmobica @michalpasztamobica (also client was released), correct?

[michalpasztamobica]

Yes, looks ready from our side.

[bulislaw]

@AnttiKauppila I'm not a big fan of APIs forcing HEAP on our users. It's a bad habit for embedded systems and as we all know can lead to issues. We spend some time in Mbed Core to make sure that use of HEAP is optional. Is lwip/sockets or networking in general requires dynamic allocation in general? If not could we rethink how this API works? If you think it's ok with the dynamic memory and it doesn't stand out from the use pattern across connectivity, could we add a warning in doxygen that it will use dynamic memory/allocate memory on the heap.

[0xc0170]

@AnttiKauppila I'm not a big fan of APIs forcing HEAP on our users. It's a bad habit for embedded systems and as we all know can lead to issues. We spend some time in Mbed Core to make sure that use of HEAP is optional. Is lwip/sockets or networking in general requires dynamic allocation in general? If not could we rethink how this API works? If you think it's ok with the dynamic memory and it doesn't stand out from the use pattern across connectivity, could we add a warning in doxygen that it will use dynamic memory/allocate memory on the heap.

@tymoteuszblochmobica @michalpasztamobica Can you answer this one?

[michalpasztamobica]

@0xc0170 , I think the final word belongs to @AnttiKauppila , so it's worth waiting for him to come back from holidays.

The dns module has and always had a lot of dynamic allocations, so Tymoteusz adjusted his changes to the existing coding style. I think completely giving up dynamic allocation could become a waste of memory as we would have to allocate all buffers as static globals shared between functions, while now we can allocate only as much as is needed.
Also - getaddrinfo is meant to allocate the memory for the caller and that is how the POSIX implementation works. This is because the number of results is unknown to the caller and is returned as a list.

I see some dynamically allocated variables which could potentially be moved to stack (for example here and here), if we really want to cut down on heap usage.

Finally - if the usage of heap as we know it from the standard is considered dangerous or harmful in a particular use case, it is possible for the user to implement their own allocators that will be called on new and delete and will do the job in a safe way.

[SeppoTakalo]

I don't think it is a realistic option to do a networked application without a heap.
That would force all components to use static buffers, which is very wasteful, as some of the buffers might be used only once per connection.

For example, DNS phase is done when you start your connections, makes lots of sense that all memory used by resolver is then freed, once we continue to connection phase, as there might be TLS/DTLS handshakes coming, which in turn require great amount of RAM as well.