Leverage the simplification of the IPv6 parsing primitive #8003

Taiki-San · 2018-09-05T15:31:04Z

Description

Apply the changes of PR #6293 to the IPv6 parser.
Thanks to changes introduced by PelionIoT/nanostack-libservice#72, we can now report the success or failure of the parsing and not rely on simply returning an empty string on failure.
This PR shouldn't be merged before PelionIoT/nanostack-libservice#72 is merged and integrated into mbedOS.

Pull request type

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

kjbracey

Looks fine, for once LibService has been updated.

NirSonnenschein · 2018-09-13T06:45:46Z

@Taiki-San , looks like there is a build issue, please take a look:

Compile [ 69.1%]: SocketAddress.cpp

[Error] SocketAddress.cpp@63,30: could not convert 'stoip6(addr, strlen(addr), ((void*)((uint8_t*)(&((SocketAddress*)this)->SocketAddress::_addr.nsapi_addr::bytes))))' from 'void' to 'bool'

[Warning] SocketAddress.cpp@70,1: control reaches end of non-void function [-Wreturn-type]

[ERROR] ./features/netsocket/SocketAddress.cpp: In member function 'bool SocketAddress::set_ip_address(const char*)':

./features/netsocket/SocketAddress.cpp:63:30: error: could not convert 'stoip6(addr, strlen(addr), ((void*)((uint8_t*)(&((SocketAddress*)this)->SocketAddress::_addr.nsapi_addr::bytes))))' from 'void' to 'bool'

Taiki-San · 2018-09-13T06:52:46Z

@NirSonnenschein That's expected from the missing PR: stoip6 signature has been changed and its return value turned from void to bool.

NirSonnenschein · 2018-09-13T06:55:30Z

@Taiki-San , sorry, I missed that interface change. restored "needs: review" status.

kjbracey · 2018-09-13T08:45:24Z

Part of me wants to give this a red cross because of you verbing the word "leverage", but resisting temptation.

Taiki-San · 2018-09-13T08:48:37Z

Ah ah, sorry about that. Do you have a better suggestion for posterity?

kjbracey · 2018-09-13T08:49:08Z

No, you do you. Not totally serious :)

0xc0170 · 2018-09-24T12:27:12Z

This PR shouldn't be merged before PelionIoT/nanostack-libservice#72 is merged and integrated into mbedOS.

@kjbracey-arm when this fix can move forward?

kjbracey · 2018-09-24T12:31:46Z

When the next libService subtree merge comes in. That's normally done as part of the once-per-release Nanostack update, but we could do an extra one because of this. @artokin, @deepakvenugopal ?

kjbracey · 2018-09-25T13:43:57Z

Waiting for #8245. Also needs a conflict resolution rebase.

Taiki-San · 2018-10-11T09:04:58Z

For the record, this PR is waiting for the next libService merge.

cmonr · 2018-10-19T19:52:33Z

@Taiki-San Looks like #8245 is now in. Was this what you mentioned with libService or is that something else?

Taiki-San · 2018-10-19T19:56:28Z

Not really involved in the process but if it wasn't reverted, #8245 contains the code this PR is depending on. Tests should run fine this time.

cmonr · 2018-10-19T20:09:39Z

@Taiki-San You might want to take a look at the Travis CI tests. I've restarted them just in case, but the errors looks legit.

Taiki-San · 2018-10-19T20:14:08Z

I don't have access to the internal CI, but this PR couldn't build until libservice was merged (a function signature changed).
Hopefully, we shouldn't get any more failures.

kjbracey · 2018-10-22T12:06:56Z

There's a unit test stub for stoip6 that blew up. Seems there's a dummy definition of stoip6 without the return value in UNITTESTS/target_h/ip6string.h.

Taiki-San · 2018-10-22T12:34:52Z

Thanks, just patched it.

cmonr · 2018-10-22T16:50:40Z

@Taiki-San Thanks. Manually restarted unittests since GitHub's webhooks are disabled right now.

Results should be ready in about 20 mins.

Taiki-San · 2018-10-22T17:29:30Z

jenkins-ci/unittests is reporting a failure, is that normal?

cmonr · 2018-10-22T18:21:10Z

@Taiki-San Nope.
This command will run the unittests: python ./UNITTESTS/mbed_unittest.py --compile --coverage both

Please take a look at the failure.

The log: **[linux] test 4
[linux] 
[linux]       Start  4: features-netsocket-NetworkStack
[linux] 
[linux] 
[linux] 
[linux] 4: Test command: /builds/ws/mbed-os-ci_unittests/unitTest-531/mbed-os/build/features-netsocket-NetworkStack
[linux] 
[linux] 4: Test timeout computed to be: 9.99988e+06
[linux] 
[linux] 4: Running main() from gmock_main.cc
[linux] 
[linux] 4: [==========] Running 13 tests from 1 test case.
[linux] 
[linux] 4: [----------] Global test environment set-up.
[linux] 
[linux] 4: [----------] 13 tests from TestNetworkStack
[linux] 
[linux] 4: [ RUN      ] TestNetworkStack.constructor
[linux] 
[linux] 4: [       OK ] TestNetworkStack.constructor (0 ms)
[linux] 
[linux] 4: [ RUN      ] TestNetworkStack.get_ip_address_default
[linux] 
[linux] 4: [       OK ] TestNetworkStack.get_ip_address_default (0 ms)
[linux] 
[linux] 4: [ RUN      ] TestNetworkStack.gethostbyname
[linux] 
[linux] 4: /builds/ws/mbed-os-ci_unittests/unitTest-531/mbed-os/UNITTESTS/features/netsocket/NetworkStack/test_NetworkStack.cpp:156: Failure
[linux] 
[linux] 4: Expected equality of these values:
[linux] 
[linux] 4:   stack->gethostbyname("host", &a, NSAPI_UNSPEC)
[linux] 
[linux] 4:     Which is: 0
[linux] 
[linux] 4:   NSAPI_ERROR_DNS_FAILURE
[linux] 
[linux] 4:     Which is: -3009
[linux] 
[linux] 4: [  FAILED  ] TestNetworkStack.gethostbyname (0 ms)
[linux] 
[linux] 4: [ RUN      ] TestNetworkStack.gethostbyname_simple_address
[linux] 
[linux] 4: [       OK ] TestNetworkStack.gethostbyname_simple_address (0 ms)
[linux] 
[linux] 4: [ RUN      ] TestNetworkStack.gethostbyname_simple_address_right_version
[linux] 
[linux] 4: [       OK ] TestNetworkStack.gethostbyname_simple_address_right_version (0 ms)
[linux] 
[linux] 4: [ RUN      ] TestNetworkStack.gethostbyname_simple_address_wrong_version
[linux] 
[linux] 4: [       OK ] TestNetworkStack.gethostbyname_simple_address_wrong_version (0 ms)
[linux] 
[linux] 4: [ RUN      ] TestNetworkStack.gethostbyname_empty_host
[linux] 
[linux] 4: [       OK ] TestNetworkStack.gethostbyname_empty_host (0 ms)
[linux] 
[linux] 4: [ RUN      ] TestNetworkStack.gethostbyname_async_delay
[linux] 
[linux] 4: /builds/ws/mbed-os-ci_unittests/unitTest-531/mbed-os/UNITTESTS/features/netsocket/NetworkStack/test_NetworkStack.cpp:193: Failure
[linux] 
[linux] 4: Expected equality of these values:
[linux] 
[linux] 4:   stack->gethostbyname_async("localhost", mbed::callback(my_callback), NSAPI_UNSPEC)
[linux] 
[linux] 4:     Which is: 0
[linux] 
[linux] 4:   NSAPI_ERROR_DNS_FAILURE
[linux] 
[linux] 4:     Which is: -3009
[linux] 
[linux] 4: mbed assertation failed: _ops, file: /builds/ws/mbed-os-ci_unittests/unitTest-531/mbed-os/UNITTESTS/../platform/Callback.h, line 1818 
[linux] 
[linux]  4/37 Test  #4: features-netsocket-NetworkStack .........................***Exception: SegFault  0.29 sec **

Taiki-San · 2018-10-22T19:50:47Z

That's weird, I can't reproduce this error on my computer. I'm running macOS/clang.
Moreover, after having a look at the test case that was failing, I'm not quite sure how this PR could be affecting its outcome...

cmonr · 2018-10-23T00:42:34Z

@ARMmbed/mbed-os-test Would y'all be able to help out here?

kjbracey · 2018-10-23T08:08:33Z

Those unit tests are checking response to bad IP addresses. Presumably would have worked when the "is it an IPv6 address" tests were inside NetworkStack.cpp, but now it's using stubbed stoip6 which always returns success?

No, actually you aren't returning anything in ip6string.h.

I see that the cmake file for the test is actually pulling in real stoip4.c -you could also pull in real stoip6.c for consistency. (And then if you're doing that, remove the dummies from ip6string_h?)

Taiki-San · 2018-10-23T09:11:37Z

Oh, so that's why it's randomly succeeding! It's basically just using whatever was on the stack/register as the success indicator.
I removed the dummy header and added the libip6string files to any target that was building libip4string. It's working, but I don't think building ip6tos.c is strictly necessary. Should I go forward with it nevertheless?

kjbracey · 2018-10-23T09:24:49Z

I don't have a view on what the correct thing to do for those tests is, except that ip4 and ip6 should be consistent. It would seem natural to leave the working ip4 as-is and bring ip6 inline.

…a valid IPv6 address

Taiki-San · 2018-10-23T09:27:33Z

Ok, I kept libip6string consistent with libip4string.

cmonr · 2018-10-24T02:54:53Z

/morph build

mbed-ci · 2018-10-24T03:41:21Z

Build : SUCCESS

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

Triggering tests

/morph test
/morph export-build
/morph mbed2-build

mbed-ci · 2018-10-24T04:44:11Z

Exporter Build : SUCCESS

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

mbed-ci · 2018-10-24T05:12:32Z

Test : FAILURE

Build number : 3239
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/8003/3239

kjbracey · 2018-10-24T07:11:02Z

Apparently spurious test failure on K82F-GCC_ARM.tests-mbed_hal-qspi.qspi frequency setting test

0xc0170 · 2018-10-24T08:56:37Z

/morph test
/morph mbed2-build

mbed-ci · 2018-10-24T10:31:02Z

Test : SUCCESS

Build number : 3241
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/8003/3241

0xc0170 · 2018-11-05T09:16:08Z

Depends on #8245 (moved to 5.11)

cmonr added needs: review needs: preceding PR labels Sep 5, 2018

kjbracey approved these changes Sep 12, 2018

View reviewed changes

NirSonnenschein added needs: work and removed needs: review labels Sep 13, 2018

NirSonnenschein added needs: review and removed needs: work labels Sep 13, 2018

cmonr added needs: work and removed needs: review labels Oct 19, 2018

cmonr added needs: CI and removed needs: preceding PR needs: work labels Oct 19, 2018

cmonr approved these changes Oct 19, 2018

View reviewed changes

cmonr added release-version: 5.10.3 needs: work and removed needs: CI labels Oct 19, 2018

Use the parsing status as a way to detect if the IP string represent …

3bc33f6

…a valid IPv6 address

cmonr added needs: CI and removed needs: work labels Oct 23, 2018

0xc0170 added ready for merge and removed needs: CI labels Oct 24, 2018

0xc0170 merged commit 7cd1478 into ARMmbed:master Oct 24, 2018

0xc0170 removed the ready for merge label Oct 24, 2018

Taiki-San deleted the patch-2 branch October 24, 2018 15:31

0xc0170 added release-version: 5.11.0-rc1 and removed release-version: 5.10.3 labels Nov 5, 2018

0xc0170 mentioned this pull request Nov 5, 2018

Release candidate #8637

Closed

Leverage the simplification of the IPv6 parsing primitive #8003

Leverage the simplification of the IPv6 parsing primitive #8003

Uh oh!

Conversation

Taiki-San commented Sep 5, 2018

Description

Pull request type

Uh oh!

kjbracey left a comment

Choose a reason for hiding this comment

Uh oh!

NirSonnenschein commented Sep 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Taiki-San commented Sep 13, 2018

Uh oh!

NirSonnenschein commented Sep 13, 2018

Uh oh!

kjbracey commented Sep 13, 2018

Uh oh!

Taiki-San commented Sep 13, 2018

Uh oh!

kjbracey commented Sep 13, 2018

Uh oh!

0xc0170 commented Sep 24, 2018

Uh oh!

kjbracey commented Sep 24, 2018

Uh oh!

kjbracey commented Sep 25, 2018

Uh oh!

Taiki-San commented Oct 11, 2018

Uh oh!

cmonr commented Oct 19, 2018

Uh oh!

Taiki-San commented Oct 19, 2018

Uh oh!

cmonr commented Oct 19, 2018

Uh oh!

Taiki-San commented Oct 19, 2018

Uh oh!

kjbracey commented Oct 22, 2018

Uh oh!

Taiki-San commented Oct 22, 2018

Uh oh!

cmonr commented Oct 22, 2018

Uh oh!

Taiki-San commented Oct 22, 2018

Uh oh!

cmonr commented Oct 22, 2018

Uh oh!

Taiki-San commented Oct 22, 2018

Uh oh!

cmonr commented Oct 23, 2018

Uh oh!

kjbracey commented Oct 23, 2018

Uh oh!

Taiki-San commented Oct 23, 2018

Uh oh!

kjbracey commented Oct 23, 2018

Uh oh!

Taiki-San commented Oct 23, 2018

Uh oh!

cmonr commented Oct 24, 2018

Uh oh!

mbed-ci commented Oct 24, 2018

Build : SUCCESS

Triggering tests

Uh oh!

mbed-ci commented Oct 24, 2018

Exporter Build : SUCCESS

Uh oh!

mbed-ci commented Oct 24, 2018

Test : FAILURE

Uh oh!

kjbracey commented Oct 24, 2018

Uh oh!

0xc0170 commented Oct 24, 2018

Uh oh!

mbed-ci commented Oct 24, 2018

Test : SUCCESS

NirSonnenschein commented Sep 13, 2018 •

edited

Loading