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.

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

Correct return value of nsapi_dns_query_multiple #5945

Merged
merged 1 commit into from Jan 29, 2018

Conversation

Projects
None yet
7 participants
@kjbracey-arm
Contributor

kjbracey-arm commented Jan 26, 2018

Documentation states that nsapi_dns_query_multiple returns the number of
addresses found on success - it was returning 0.

Overloads using SocketAddress are relying on the return value, meaning
those calls didn't work at all.

Fixes #5921.

Correct return value of nsapi_dns_query_multiple
Documentation states that nsapi_dns_query_multiple returns the number of
addresses found on success - it was returning 0.

Overloads using SocketAddress are relying on the return value, meaning
those calls didn't work at all.

Fixes #5921.

@0xc0170 0xc0170 requested a review from mikaleppanen Jan 26, 2018

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:dns_multiple branch from 8029dea to 15a3922 Jan 26, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jan 26, 2018

/morph build

@kFYatek

Looks great to me, just one comment.

@@ -243,8 +243,9 @@ static nsapi_size_or_error_t nsapi_dns_query_multiple(NetworkStack *stack, const
}
const uint8_t *response = packet;
if (dns_scan_response(&response, addr, addr_count) > 0) {
result = NSAPI_ERROR_OK;
int count = dns_scan_response(&response, addr, addr_count);

This comment has been minimized.

@kFYatek

kFYatek Jan 26, 2018

You might consider changing the return type of dns_scan_response(). It currently returns an int, even though its main return path is return count; and count is an unsigned.

Seems pretty fishy if you ask me. On the other hand, this function returns nsapi_size_or_error_t anyway, and that's an alias to signed int, so the cast from unsigned to int will happen somewhere anyway, so I guess this point is just purely stylistic.

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Jan 26, 2018

Contributor

Yep, we've got to get from addr_count being unsigned ultimately back to that nsapi_size_or_error_t. I don't think it's worth worrying about - any changes are liable to trigger new "comparison of signed and unsigned" warning. Let's keep this change minimal and it can slip into a patch release.

@mbed-ci

This comment has been minimized.

mbed-ci commented Jan 26, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jan 26, 2018

/morph export-build

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@orenc17

This comment has been minimized.

Contributor

orenc17 commented Jan 28, 2018

/morph uvisor-test

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Jan 29, 2018

@cmonr cmonr merged commit 49cdb0b into ARMmbed:master Jan 29, 2018

19 checks passed

ARM mbed CI Verification build successful.
Details
AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Local events testing has passed
Details
travis-ci/littlefs Local littlefs testing has passed
Details
travis-ci/mbed2-ATMEL Local mbed2-ATMEL testing has passed
Details
travis-ci/mbed2-MAXIM Local mbed2-MAXIM testing has passed
Details
travis-ci/mbed2-NORDIC Local mbed2-NORDIC testing has passed
Details
travis-ci/mbed2-NUVOTON Local mbed2-NUVOTON testing has passed
Details
travis-ci/mbed2-NXP Local mbed2-NXP testing has passed
Details
travis-ci/mbed2-RENESAS Local mbed2-RENESAS testing has passed
Details
travis-ci/mbed2-SILICON_LABS Local mbed2-SILICON_LABS testing has passed
Details
travis-ci/mbed2-STM Local mbed2-STM testing has passed
Details
travis-ci/tools Local tools testing has passed
Details

@kjbracey-arm kjbracey-arm deleted the kjbracey-arm:dns_multiple branch Jan 30, 2018

@cmonr cmonr removed the ready for merge label Jan 31, 2018

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