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

Fixed ppp_lwip_disconnect hangs when connection failure. #7160

Merged
merged 2 commits into from Jun 11, 2018

Conversation

Projects
None yet
6 participants
@jarvte
Contributor

jarvte commented Jun 7, 2018

Description

  • fixed ppp_lwip_disconnect hangs when connection failure.
  • improved nsapi_ppp_connect error handling
  • fixing issue #6979

@AriParkkila @kjbracey-arm please review.

Internal ref to defect: IOTCELL-1028

Pull request type

[X] Fix
[ ] Refactor
[ ] New target
[ ] Feature
[ ] Breaking change
@jarvte

This comment has been minimized.

Contributor

jarvte commented Jun 7, 2018

@0xc0170 travis failure not related to this fix, probably ci/pr-head neither.

my_stream = NULL;
/* close call made, now let's catch the response in the status callback */
sys_arch_sem_wait(&ppp_close_sem, 0);
ppp_active = false;

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Jun 7, 2018

Contributor

Probably should actually set ppp_active to false even if ppp_close returns an error. It's probably down if that happens.

This comment has been minimized.

@jarvte

jarvte Jun 8, 2018

Contributor

true, fixed

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 7, 2018

@jarvte Thanks for that comment. Restarted that Travis job...

if (ppp_active) {
err_t ret = ppp_close(my_ppp_pcb, 0);
if (ret != ERR_OK) {
ppp_active = false;

This comment has been minimized.

@AriParkkila

AriParkkila Jun 8, 2018

Contributor

All paths set ppp_active = false;, could be done once?

This comment has been minimized.

@jarvte

jarvte Jun 8, 2018

Contributor

ppp_active can't be called before sys_arch_sem_wait but did some modifications.

@@ -373,6 +372,9 @@ nsapi_error_t nsapi_ppp_connect(FileHandle *stream, Callback<void(nsapi_event_t,
retcode = lwip._add_ppp_interface(stream, true, stack, &my_interface);
if (retcode != NSAPI_ERROR_OK) {
my_interface = NULL;
my_stream->set_blocking(true);
my_stream = NULL;
connection_status_cb = NULL;

This comment has been minimized.

@AriParkkila

AriParkkila Jun 8, 2018

Contributor

AT_CellularNetwork::open_data_channel should release file handle before calling nsapi_ppp_connect(), and also reinitialise file handle if nsapi_ppp_connect() returns failure.

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Jun 8, 2018

Contributor

By "release" do you mean unset its sigio callback? Yes, probably wise.

This comment has been minimized.

@jarvte

jarvte Jun 8, 2018

Contributor

no need to do it here, only after my_interface->bringup which will set sigio. Added unset of sigio there. Good point!

my_stream->set_blocking(true);
my_stream = NULL;
return retcode;

This comment has been minimized.

@AriParkkila

AriParkkila Jun 8, 2018

Contributor

AT_CellularNetwork::disconnect should probably take ownership also in case of if (err != NSAPI_ERROR_OK)

This comment has been minimized.

@jarvte

jarvte Jun 8, 2018

Contributor

true, fixed to AT_Cellularnetwork::disconnect()

@jarvte jarvte force-pushed the jarvte:ppp_lwip_disconnect_hangs branch from a9bec43 to db5f38b Jun 8, 2018

Review fix:
- set ppp_active false if close fails in ppp disconnect.
- unset sigio in ppp disconnect
- take ownership of filehandle in CellularNetwork::disconnect even in case of failure

@jarvte jarvte force-pushed the jarvte:ppp_lwip_disconnect_hangs branch from db5f38b to b35dc6a Jun 8, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 10, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Jun 10, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 11, 2018

/morph export-build
/morph mbed2-build

@mbed-ci

This comment has been minimized.

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Jun 11, 2018

@jarvte

This comment has been minimized.

Contributor

jarvte commented Jun 11, 2018

@0xc0170 please merge

@cmonr cmonr merged commit de99c0f into ARMmbed:master Jun 11, 2018

14 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build 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/astyle Passed, 920 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9794 cycles (+965 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 9964B (+0.00%)
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details

@jarvte jarvte deleted the jarvte:ppp_lwip_disconnect_hangs branch Jun 20, 2018

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