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

TLSSocketWrapper::recvfrom sets SocketAddress output variable #11169

Merged
merged 1 commit into from Aug 20, 2019

Conversation

@dmaziec1
Copy link
Contributor

commented Aug 6, 2019

Description

According to: #10865 . In TLSSocketWrapper::recvfrom outputs SocketAddress the peer address. It is tested by UNITTEST. TLSSocketWrapper unittest does not use stoip4_stub anymore.

Pull request type

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

Reviewers

@SeppoTakalo
@michalpasztamobica
@kjbracey-arm

@dmaziec1 dmaziec1 force-pushed the dmaziec1:TLSSocketWrapper_recvfrom_modified branch from 31b1a28 to 9d3fd32 Aug 6, 2019

@ciarmcom ciarmcom requested review from kjbracey-arm, michalpasztamobica, SeppoTakalo and ARMmbed/mbed-os-maintainers Aug 6, 2019

@ciarmcom

This comment has been minimized.

Copy link
Member

commented Aug 6, 2019

@SeppoTakalo
Copy link
Contributor

left a comment

Fix looks OK, but I would prefer not to copy the stoip4() into the stub folder.
If we require the original code in this case, we should include it in the test.

* \param dest buffer for address. MUST be 4 bytes.
* \return boolean set to true if conversion succeded, false if it didn't
*/
bool stoip4(const char *ip4addr, size_t len, void *dest)
{

This comment has been minimized.

Copy link
@SeppoTakalo

SeppoTakalo Aug 6, 2019

Contributor

Instead of copying the stoip4() function here, can we modify the build scripts to include the original code.

Maintaining a stub and original, which both are identical, does not make sense.

This comment has been minimized.

Copy link
@michalpasztamobica

michalpasztamobica Aug 6, 2019

Contributor

It is not exactly the original. It never returns false and doesn't even perform all the checks, so it can be safely used in other tests, which don't really care about the data being copied or not, and would do fine with an empty stub.
However, to check that the getpeername works we need to perform the copy in this one place - hence the copying algorithm runs if the inputs are valid.
If we were to use the actual function we would have to guarantee the input parameters validity. Should we?

This comment has been minimized.

Copy link
@SeppoTakalo

SeppoTakalo Aug 6, 2019

Contributor

If I remove the usage of this stub from TLSSocket tests, it passes OK.

$ git diff
diff --git i/UNITTESTS/features/netsocket/TLSSocketWrapper/unittest.cmake w/UNITTESTS/features/netsocket/TLSSocketWrapper/unittest.cmake
index 2ff13c2dc6..d126328baf 100644
--- i/UNITTESTS/features/netsocket/TLSSocketWrapper/unittest.cmake
+++ w/UNITTESTS/features/netsocket/TLSSocketWrapper/unittest.cmake
@@ -28,8 +28,6 @@ set(unittest-test-sources
   stubs/mbed_shared_queues_stub.cpp
   stubs/nsapi_dns_stub.cpp
   stubs/EventFlags_stub.cpp
-  stubs/stoip4_stub.c
-  stubs/ip4tos_stub.c
   stubs/SocketStats_Stub.cpp
 )

Notice that UNITTESTS/features/netsocket/TLSSocketWrapper/unittest.cmake already seem to include the original stoip4()

I does not seem to require getpeername(), that is tested elsewhere.

This comment has been minimized.

Copy link
@dmaziec1

dmaziec1 Aug 6, 2019

Author Contributor

Thanks, good point. We haven't tried it before, but it works.

TLSSocketWrapper::recvfrom sets SocketAddress output variable
UNITTEST added. TLSSocketWrapper unittest does not use stoip4_stub anymore.

@dmaziec1 dmaziec1 force-pushed the dmaziec1:TLSSocketWrapper_recvfrom_modified branch from 9d3fd32 to e9059c2 Aug 6, 2019

@0xc0170
0xc0170 approved these changes Aug 9, 2019

@0xc0170 0xc0170 added needs: review and removed needs: work labels Aug 9, 2019

@0xc0170 0xc0170 requested a review from SeppoTakalo Aug 9, 2019

@michalpasztamobica

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2019

@0xc0170 , I cannot rerun the failed job, but I believe if you do, we will be able to merge this, as it's reviewed and approved.
Unless, @kjbracey-arm , would like to have a final word, as the original author of the issue report? :)

@0xc0170

This comment has been minimized.

Copy link
Member

commented Aug 19, 2019

CI started

@0xc0170

This comment has been minimized.

Copy link
Member

commented Aug 19, 2019

continuous-integration/jenkins/pr-merge

Should be fixed as well :)

@mbed-ci

This comment has been minimized.

Copy link

commented Aug 19, 2019

Test run: SUCCESS

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

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Aug 20, 2019

@0xc0170 0xc0170 merged commit 64fb49e into ARMmbed:master Aug 20, 2019

27 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/jenkins/pr-merge Manually fixed status
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 RTOS ROM(+0 bytes) RAM(+0 bytes)
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 8643 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.