openthread / ot-commissioner

OpenThread Commissioner, a Thread commissioner for joining new Thread devices and managing Thread networks.
https://openthread.io/
BSD 3-Clause "New" or "Revised" License
52 stars 35 forks source link

[library] fix commissioner state when sending keep-alive failed #149

Closed wgtdkp closed 4 years ago

wgtdkp commented 4 years ago

This PR fixes commissioner state transition when we failed to send the Keep-Alive message:

  1. disconnect from the Border Agent.
  2. Set the commissioner state to kDisabled when disconnected.
codecov-io commented 4 years ago

Codecov Report

Merging #149 into master will decrease coverage by 0.02%. The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master     #149      +/-   ##
==========================================
- Coverage   70.11%   70.09%   -0.03%     
==========================================
  Files          52       52              
  Lines        4752     4751       -1     
==========================================
- Hits         3332     3330       -2     
- Misses       1420     1421       +1     
Impacted Files Coverage Δ
src/library/commissioner_impl.cpp 75.76% <50.00%> (-0.07%) :arrow_down:
include/commissioner/commissioner.hpp 43.75% <0.00%> (-2.92%) :arrow_down:
src/app/json.cpp 52.86% <0.00%> (-0.39%) :arrow_down:
wgtdkp commented 4 years ago

LGTM 👍

May I ask when would it happen that the commissioner failed to send KA ?

@simonlingoogle The case I saw is in CCM mode. The Keep-Alive message is required to be signed and it may fail when an invalid Commissioner Token is used (this is allowed for certification). https://github.com/openthread/ot-commissioner/blob/5cfcb390c5c0a7896974fcb6fb906b0733109cc1/src/library/commissioner_impl.cpp#L1302-L1305

There are also other cases that may fail this method (e.g. appending to the CoAP message failed) although they are very unusual.

simonlingoogle commented 4 years ago

LGTM 👍 May I ask when would it happen that the commissioner failed to send KA ?

@simonlingoogle The case I saw is in CCM mode. The Keep-Alive message is required to be signed and it may fail when an invalid Commissioner Token is used (this is allowed for certification). https://github.com/openthread/ot-commissioner/blob/5cfcb390c5c0a7896974fcb6fb906b0733109cc1/src/library/commissioner_impl.cpp#L1302-L1305

There are also other cases that may fail this method (e.g. appending to the CoAP message failed) although they are very unusual.

Thanks for explanation. It makes sense to me to resign if sending KA failed.