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.

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

Nanostack: translate errors from sendmsg #10410

Merged

Conversation

Projects
None yet
7 participants
@michalpasztamobica
Copy link
Contributor

commented Apr 16, 2019

Description

Translate errors to NSAPI_ERROR_*, instead of always returning NSAPI_ERROR_DEVICE_ERROR.

Originally, I only wanted to add the handling of -2 (out of memory) error, to fix the numerous SENDTO_REPEAT failures I can see in Nanostack, but while I was at it I though it is wise to translate the other errors as well, instead of just returning DEVICE_ERROR and loosing the information returned from sendmsg call.

Pull request type

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

Reviewers

@SeppoTakalo
@kjbracey-arm
@KariHaapalehto
@VeijoPesonen

Release Notes

Nanostack send calls return a more accurate information in case of errors.

@michalpasztamobica michalpasztamobica changed the title Nanostak: translate errors from sendmsg Nanostack: translate errors from sendmsg Apr 16, 2019

@@ -648,6 +648,18 @@ nsapi_size_or_error_t Nanostack::do_sendto(void *handle, const ns_address_t *add
* */
if (retcode == NS_EWOULDBLOCK) {
ret = NSAPI_ERROR_WOULD_BLOCK;
} else if (retcode == -1) {
ret = NSAPI_ERROR_PARAMETER;

This comment has been minimized.

Copy link
@SeppoTakalo

SeppoTakalo Apr 16, 2019

Contributor

If we refer to Nanostack's socket API:

/**
 ...
 * Error returns:
 *
 * \return -1 Invalid socket ID.
 * \return -2 Socket memory allocation fail.
 * \return -3 TCP state not established or address scope not defined .
 * \return -4 Unknown interface.
 * \return -5 Socket not connected
 * \return -6 Packet too short (ICMP raw socket error).
 */
int16_t socket_sendmsg(int8_t socket, const ns_msghdr_t *msg, int flags);

then should -1 be NSAPI_ERROR_NO_SOCKET

This comment has been minimized.

Copy link
@michalpasztamobica

michalpasztamobica Apr 16, 2019

Author Contributor

This is how -1 is returned:

    if (!msg) {
        return -1;
    }

Or here (I think this is the one they meant by "Invalid socket ID."):

    if (!socket_ptr || !socket_ptr->inet_pcb) {
        tr_warn("Socket Id %i", sid);
        ret_val = -1;
        goto fail;
    }

Or here:

        if (!socket_message_validate_iov(msg, &payload_length)) {
            return -1;
        }

Looks to me like -1 is returned if the input parameters are wrong.

This comment has been minimized.

Copy link
@SeppoTakalo

SeppoTakalo Apr 16, 2019

Contributor

OK. Then its clearly a parameter error.

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Apr 16, 2019

Contributor

Yes, -1 is generally "parameter error" in Nanostack, for the few socket functions that have multiple error returns. But the socket ID isn't distinguished from any other faulty parameter.

I would have thought there would be scope for NSAPI_ERROR_NO_SOCKET being generated in the wrapper, at least.

@ciarmcom ciarmcom requested review from KariHaapalehto, kjbracey-arm, SeppoTakalo, VeijoPesonen and ARMmbed/mbed-os-maintainers Apr 16, 2019

@ciarmcom

This comment has been minimized.

@@ -648,6 +648,18 @@ nsapi_size_or_error_t Nanostack::do_sendto(void *handle, const ns_address_t *add
* */
if (retcode == NS_EWOULDBLOCK) {
ret = NSAPI_ERROR_WOULD_BLOCK;
} else if (retcode == -1) {
ret = NSAPI_ERROR_PARAMETER;

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Apr 16, 2019

Contributor

Yes, -1 is generally "parameter error" in Nanostack, for the few socket functions that have multiple error returns. But the socket ID isn't distinguished from any other faulty parameter.

I would have thought there would be scope for NSAPI_ERROR_NO_SOCKET being generated in the wrapper, at least.

@@ -648,6 +648,18 @@ nsapi_size_or_error_t Nanostack::do_sendto(void *handle, const ns_address_t *add
* */
if (retcode == NS_EWOULDBLOCK) {
ret = NSAPI_ERROR_WOULD_BLOCK;
} else if (retcode == -1) {

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Apr 16, 2019

Contributor

Feels like this is getting to the point it should be a switch statement for the error cases, although I guess you still need an if for the < 0.

This comment has been minimized.

Copy link
@michalpasztamobica

michalpasztamobica Apr 16, 2019

Author Contributor

Yes, that's why I stayed with it if-else statements.

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Apr 16, 2019

Contributor

Just a tad wary that the compiler will end up doing all the comparisons in the order specified. Compilers are generally good at spotting how to break a switch up into ranges and figuring out when to do a branch table, and when to do binary chop, but I'm not sure how good they are at optimising an if-else chain on 1 variable.

At least make the non-error case the first, if going down this route, for speed.

It might be time to use a non-conventional if-else-switch control construct:

if (retcode >= 0) {
   ret = retcode;
} else switch (retcode) {
   case -1:
       ret = NSAPI_ERROR_PARAMETER;
       break;
   ...
   default:
       tr_error();
       ret = NSAPI_ERROR_DEVICE_ERROR;
       break;
}

(Would look neater if Mbed OS style didn't insist on the extra switch indent).

This comment has been minimized.

Copy link
@michalpasztamobica

michalpasztamobica Apr 16, 2019

Author Contributor

Done

@michalpasztamobica michalpasztamobica force-pushed the michalpasztamobica:nanostack_translate_sendmsg_errors branch from 69083d4 to eb74e45 Apr 16, 2019

tr_error("socket_sendmsg: error=%d", retcode);
ret = NSAPI_ERROR_DEVICE_ERROR;
} else {
if (retcode >=0 ) {

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Apr 16, 2019

Contributor

Stray space that the formatter will likely complain about

Nanostack: translate errors from sendmsg
Translate errors to NSAPI_ERROR_*, instead of always returning
NSAPI_ERROR_DEVICE_ERROR

@michalpasztamobica michalpasztamobica force-pushed the michalpasztamobica:nanostack_translate_sendmsg_errors branch from eb74e45 to d1d0118 Apr 16, 2019

@mbed-ci

This comment has been minimized.

Copy link

commented Apr 17, 2019

Test run: FAILED

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

Failed test jobs:

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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2019

Failures are in components-storage-blockdevice-component_sd-tests-* , rather irrelevant for this PR.

@0xc0170

This comment has been minimized.

Copy link
Member

commented Apr 17, 2019

Test restarted

@adbridge adbridge added needs: CI and removed needs: review labels Apr 25, 2019

@adbridge

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

ci started

@mbed-ci

This comment has been minimized.

Copy link

commented Apr 26, 2019

Test run: SUCCESS

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

@adbridge

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

Marking this for 5.13 as it looks like an interface change to me. @michalpasztamobica could you please also add a release notes section to the header explaining the impact to the user.

@michalpasztamobica

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2019

@adbridge , I would say this is more of a fix - after all the function call looks the same and the return type does not change either. We only fix the code to return appropriate error values.

I will leave it up to you to decide - the Release Note is added.

@adbridge

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

@michalpasztamobica My concern would be that if a user of the interface is just expecting to receive the one previous error code but now could get multiple different ones, their application could fail ?

@michalpasztamobica

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2019

@adbridge , I misunderstood "interface change" to be a PR that will actually change the API (like add a new function, a parameter or change return type). But giving it a second thought - you are right: anything that can become an annoyance to the users better be communicated clearly in the release notes.

@adbridge adbridge merged commit 9f18456 into ARMmbed:master Apr 26, 2019

26 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage Success
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Success!
Details
travis-ci/docs Success!
Details
travis-ci/doxy-spellcheck Success!
Details
travis-ci/events Success! Runtime is 8732 cycles.
Details
travis-ci/gitattributestest Success!
Details
travis-ci/include_check Success!
Details
travis-ci/licence_check Success!
Details
travis-ci/littlefs Success! Code size is 8448B.
Details
travis-ci/psa-autogen Success!
Details
travis-ci/tools-py2.7 Success!
Details
travis-ci/tools-py3.5 Success!
Details
travis-ci/tools-py3.6 Success!
Details
travis-ci/tools-py3.7 Success!
Details
@SeppoTakalo

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2019

Application that expects certain kind of errors, sound a bit unstable in a first place.

Being portable, application should be able to fail gracefully in any error, and work normally between all connectivities we offer.

To me this change brings clarify to Nanostack's error codes, but Anna's concern is valid, and we should therefore release this only in 5.13.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.