Skip to content

Add Getaddrinfo interface for multiple DNS adresses#11653

Merged
0xc0170 merged 1 commit into
ARMmbed:masterfrom
tymoteuszblochmobica:multiple
Jan 7, 2020
Merged

Add Getaddrinfo interface for multiple DNS adresses#11653
0xc0170 merged 1 commit into
ARMmbed:masterfrom
tymoteuszblochmobica:multiple

Conversation

@tymoteuszblochmobica

@tymoteuszblochmobica tymoteuszblochmobica commented Oct 8, 2019

Copy link
Copy Markdown
Contributor

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

ciarmcom commented Oct 8, 2019

Copy link
Copy Markdown
Member

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

@SeppoTakalo

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor Author

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

tymoteuszblochmobica commented Oct 14, 2019

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
Contributor

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

Most users don't specify the interface.

@tymoteuszblochmobica

Copy link
Copy Markdown
Contributor Author

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

@tymoteuszblochmobica tymoteuszblochmobica force-pushed the multiple branch 9 times, most recently from 0fbd134 to e2b06ba Compare October 17, 2019 15:17
}
}

SocketAddress *temp = new (std::nothrow) SocketAddress [MBED_CONF_NSAPI_DNS_ADDRESSES_LIMIT];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@kivaisan

Copy link
Copy Markdown
Contributor

Also update PR description gethostbyname -> getaddrinfo

@tymoteuszblochmobica

tymoteuszblochmobica commented Dec 11, 2019

Copy link
Copy Markdown
Contributor Author

Created PR to fix Cloud Client async DNS callback
https://github.com/ARMmbed/mbed-client-pal/pull/852

@0xc0170

0xc0170 commented Dec 11, 2019

Copy link
Copy Markdown
Contributor

If we provide a PR to mbed-cloud-client, modifying mbed-client-pal, we will break it until this PR is merged. Is this acceptable?

It could be as this is almost ready, just need to coordinate this breaking change.
@teetak01 Please advise

@michalpasztamobica

Copy link
Copy Markdown
Contributor

@0xc0170 , @teetak01 the internal mbed-client-pal change, which should allow the new API to work, has just been merged. I can see that the CI copies a tarball with the project rather than checking it out from github, so I can't tell if it is using the latest mbed-client-pal or the released one, present in mbed-cloud-client project?
If it's the latest from internal repository - we can restart the CI.
.If it's the release one - then we need to wait for a release in mbed-cloud-client.

@teetak01

Copy link
Copy Markdown
Contributor

It will require new release from client.

@teetak01

Copy link
Copy Markdown
Contributor

I am targeting the client fix for 4.2.0 release which should happen next week.

@tymoteuszblochmobica

Copy link
Copy Markdown
Contributor Author

It seems that Cloud Client is fixed now.
@0xc0170 Should CI be restarted?

@teetak01

Copy link
Copy Markdown
Contributor

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

@tymoteuszblochmobica

Copy link
Copy Markdown
Contributor Author

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

@michalpasztamobica

Copy link
Copy Markdown
Contributor

@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.

New members are added to the network interface
-getaddrinfo
-getaddrinfo_async
gethostbyname is unchanged but gethostbyname_async  result param now contains results od DNS records found.
Test cases for sync/async added added to DNS test folder.
@tymoteuszblochmobica

Copy link
Copy Markdown
Contributor Author

CI can be restarted

@mbed-ci

mbed-ci commented Dec 19, 2019

Copy link
Copy Markdown

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

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor

CI restarted

@mbed-ci

mbed-ci commented Dec 19, 2019

Copy link
Copy Markdown

Test run: SUCCESS

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

@0xc0170

0xc0170 commented Dec 20, 2019

Copy link
Copy Markdown
Contributor

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

@0xc0170 0xc0170 requested review from a user and bulislaw December 20, 2019 10:31
@michalpasztamobica

Copy link
Copy Markdown
Contributor

Yes, looks ready from our side.

@bulislaw bulislaw left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

0xc0170 commented Jan 3, 2020

Copy link
Copy Markdown
Contributor

@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

Copy link
Copy Markdown
Contributor

@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

Copy link
Copy Markdown
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

BREAKING-CHANGE release-version: 6.0.0-alpha-1 First pre-release version of 6.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.