Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Runtime DNS server addition implement #10602

Merged
merged 1 commit into from
May 22, 2019
Merged

Runtime DNS server addition implement #10602

merged 1 commit into from
May 22, 2019

Conversation

tymoteuszblochmobica
Copy link
Contributor

@tymoteuszblochmobica tymoteuszblochmobica commented May 16, 2019 •
edited
Loading

Description

Implemented missing runtime DNS server addition in LWIPStack class.

Pull request type

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

Reviewers

@SeppoTakalo
@mikaleppanen
@kivaisan

Release Notes

@ciarmcom
Copy link
Member

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

@andrewc-arm
Copy link
Contributor

Hi, @tymoteuszblochmobica

Thanks for making this progress!

I see that the implementation add_dns_server() will push the new DNS server to the index 0.
So, does this mean that user application can get_dns_server() and add_dns_server() to reorder the priority?
If so, can we expect the same behavior on generic NetworkInterface::add_dns_server() ?
If so, can we mention such behavior in Doxygen comment?

I am asking you this because DNS priority concept is also required. My customer's customer has a local DNS server of lower network latency and private domain name list. Or do you think this should be dealt as a separate request?

(CC: @taeily , @joslee02 , @ohadhawk )

JanneKiiskila
JanneKiiskila reviewed May 20, 2019
View reviewed changes
features/netsocket/NetworkInterface.h Outdated
@@ -253,6 +253,18 @@ class NetworkInterface: public DNS {
*/
virtual nsapi_error_t gethostbyname_async_cancel(int id);

/** Get a domain name server from a list of servers to query
*
* Returns a DNS server address for a index. If returns error no more
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit-picking, but the English here could do a bit of fine-tuning. I assume we store the server address to the @param address and also it's name to @param interface_name AND NS_API_ERROR_OK, if a DNS server is availble in the index queried location? If no DNS server in index queries location, we return some negative error code, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Member get_dns_server is removed.

SeppoTakalo
SeppoTakalo reviewed May 20, 2019
View reviewed changes
features/netsocket/NetworkInterface.h Outdated
* @param interface_name Network interface name
* @return NSAPI_ERROR_OK on success, negative error code on failure
*/
virtual nsapi_error_t get_dns_server(int index, SocketAddress *address, const char *interface_name);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is actually API break, as the size of the virtual table changes, and therefore forces us to rebuild all binary WiFi drivers that inherit NetworkInterface.

Is this new API really required?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi, @tymoteuszblochmobica
As @SeppoTakalo shared his concern on adding the new API call, can we divide this PR to add_dns_server() implementation and get_dns_server() connection separately? What customer really need urgently is add_dns_server() call.

Copy link
Contributor Author

@tymoteuszblochmobica tymoteuszblochmobica May 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

As member get_dns_server breaks API and is not urgenly needed as add_dns_server is removed.

@kivaisan
Copy link
Contributor

I'm testing this but I always get NSAPI_ERROR_NO_ADDRESS error. I already added some traces and it looks like convert_mbed_addr_to_lwip returns null.

    tr_info("add_dns_server: address = %s", address.get_ip_address());
    tr_info("convert = %d", convert_mbed_addr_to_lwip(ip_addr, &addr));
    tr_info("add_dns_server: ip_addr = %s", ipaddr_ntoa(ip_addr));

->

[00025618ms][INFO][LWIP]: add_dns_server: address = 1.1.1.1
[00025621ms][INFO][LWIP]: convert = 1
[00025622ms][INFO][LWIP]: add_dns_server: ip_addr = (null)

Any idea? I'll also do some more debugging.

kivaisan
kivaisan suggested changes May 20, 2019
View reviewed changes
features/lwipstack/LWIPStack.cpp Outdated
int index;
nsapi_addr_t addr = address.get_addr();
const ip_addr_t *ip_addr_move;
ip_addr_t *ip_addr;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be ip_addr_t ip_addr;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

features/lwipstack/LWIPStack.cpp Outdated
const ip_addr_t *ip_addr_move;
ip_addr_t *ip_addr;

convert_mbed_addr_to_lwip(ip_addr, &addr);
Copy link
Contributor

Choose a reason for hiding this comment

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

and this (and later in function) should then use &ip_addr.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@kivaisan
Copy link
Contributor

One more bug which prevents DNS usage when interface name has been given to gethostbyname method:
https://github.com/ARMmbed/mbed-os/blob/master/features/lwipstack/lwip/src/core/lwip_dns.c#L490
This comparision should be

  if (numdns >= DNS_MAX_SERVERS) {
    return IP_ADDR_ANY;
  }

@tymoteuszblochmobica
Copy link
Contributor Author

if (numdns >= DNS_MAX_SERVERS) {
return IP_ADDR_ANY;
}
Fix was already done but probably lost during rebase.
Applied again.

@0xc0170
Copy link
Contributor

0xc0170 commented May 21, 2019

CI started

@mbed-ci
Copy link

mbed-ci commented May 21, 2019

Test run: SUCCESS

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

@0xc0170 0xc0170 merged commit 06cf787 into ARMmbed:master May 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewc-arm andrewc-arm andrewc-arm left review comments

@JanneKiiskila JanneKiiskila JanneKiiskila left review comments

@0xc0170 0xc0170 0xc0170 approved these changes

@SeppoTakalo SeppoTakalo SeppoTakalo approved these changes

@kivaisan kivaisan kivaisan approved these changes

@mikaleppanen mikaleppanen Awaiting requested review from mikaleppanen

Assignees
No one assigned
Labels
release-version: 5.13.0-rc1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@tymoteuszblochmobica @ciarmcom @andrewc-arm @kivaisan @0xc0170 @mbed-ci @SeppoTakalo @JanneKiiskila