Skip to content

Conversation

bulislaw
Copy link
Member

@bulislaw bulislaw commented Oct 7, 2016

Description

DNS name resolution was returning error even for successful cases

Status

READY

CC: @geky @0xc0170

Copy link
Contributor

@0xc0170 0xc0170 left a comment

Choose a reason for hiding this comment

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

+1

@geky
Copy link
Contributor

geky commented Oct 7, 2016

There was an unrelated change to this function that slightly changed the logic. Are you sure the dns_scan_response's return value doesn't need to be checked for a valid match?

@bulislaw
Copy link
Member Author

bulislaw commented Oct 7, 2016

Yeah you're probably right, we should still return error when the count is 0.

@sg-
Copy link
Contributor

sg- commented Oct 7, 2016

/morph test

@mbed-bot
Copy link

mbed-bot commented Oct 7, 2016

Result: SUCCESS

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

/morph test

Output

mbed Build Number: 1078

All builds and test passed!

@0xc0170 0xc0170 removed the needs: CI label Oct 10, 2016
@0xc0170 0xc0170 merged commit d6389a4 into ARMmbed:master Oct 10, 2016
const uint8_t *response = packet;
dns_scan_response(&response, addr, addr_count);
break;
if (!dns_scan_response(&response, addr, addr_count)) {
Copy link
Contributor

@kjbracey kjbracey Oct 10, 2016

Choose a reason for hiding this comment

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

Don't like using "!" for non-boolean values. "== 0" would have been clearer.

Also, you end up querying every DNS server when the destination doesn't exist (like 5.1 did). The point of extra DNS servers is to provide a backup if one goes down, not to get second opinions.

And you return "OK" if no servers respond.

Better fix is:

 if (dns_scan_response(&response, addr, addr_count) > 0) {
      result = NSAPI_ERROR_OK;
 }
break;

@bulislaw bulislaw mentioned this pull request Oct 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants