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

Clean-up symettry in SessionManager #13591

Conversation

tcarmelveilleux
Copy link
Contributor

Problem

  • SessionManager has symetrical processing steps for
    group and unsecured message, but some side-effects of
    unicast secure sessions are delegated to SecureMessageCodec
    that should actually just be encrypt/decrypt logic.
  • This is needed to assist in having a single point of tracing
    for incoming/outgoing messages (upcoming PR). Current organization
    requires putting that logic in several modules due to the
    side-effects occuring at the wrong layer.

Change overview

Done by this PR:

  • Hoists those side effects (e.g. updating counter) up from
    SecureMessageCodec where they do not belong, into SessionManager.
  • Fix documentation and argument names of SecureMessageCodec that
    had rotted over many refactors and terminological changes.
  • Add a bit of processing symmetry in SessionManager

Changes are structural only, and non-functional. Should yield no change
in operation.

Testing

Testing done: unit tests pass, integration tests pass

- SessionManager has symetrical processing steps for
  group and unsecured message, but some side-effects of
  unicast secure sessions are delegated to SecureMessageCodec
  that should actually just be encrypt/decrypt logic.
- This is needed to assist in having a single point of tracing
  for incoming/outgoing messages (upcoming PR). Current organization
  requires putting that logic in several modules due to the
  side-effects occuring at the wrong layer.

Done by this PR:
- Hoists those side effects (e.g. updating counter) up from
  SecureMessageCodec where they do not belong, into SessionManager.
- Fix documentation and argument names of SecureMessageCodec that
  had rotted over many refactors and terminological changes.
- Add a bit of processing symmetry in SessionManager

Testing done: unit tests pass, integration tests pass
@github-actions
Copy link

github-actions bot commented Jan 14, 2022

PR #13591: Size comparison from 3b8a3c7 to 76cef5c

Increases (2 builds for linux)
platform target config section 3b8a3c7 76cef5c change % change
linux chip-tool-ipv6only arm64 (read only) 8038388 8038420 32 0.0
.text 6830532 6830564 32 0.0
thermostat-no-ble arm64 (read only) 2041932 2041964 32 0.0
.text 1697456 1697488 32 0.0
Decreases (1 build for telink)
platform target config section 3b8a3c7 76cef5c change % change
telink lighting-app tlsr9518adk80d (read/write) 838430 838406 -24 -0.0
text 585584 585562 -22 -0.0
Full report (14 builds for efr32, k32w, linux, p6, qpg, telink)
platform target config section 3b8a3c7 76cef5c change % change
efr32 lighting-app BRD4161A (read only) 832732 832732 0 0.0
(read/write) 127628 127628 0 0.0
.bss 125744 125744 0 0.0
.data 1884 1884 0 0.0
.text 832724 832724 0 0.0
BRD4161A+rpc (read only) 820152 820152 0 0.0
(read/write) 144288 144288 0 0.0
.bss 142304 142304 0 0.0
.data 1984 1984 0 0.0
.text 820144 820144 0 0.0
window-app BRD4161A (read only) 805392 805392 0 0.0
(read/write) 126320 126320 0 0.0
.bss 124480 124480 0 0.0
.data 1836 1836 0 0.0
.text 805384 805384 0 0.0
k32w light k32w061+release (read/write) 657888 657888 0 0.0
.bss 77136 77136 0 0.0
.data 1852 1852 0 0.0
.text 573100 573100 0 0.0
lock k32w061+release (read/write) 661572 661572 0 0.0
.bss 77432 77432 0 0.0
.data 1872 1872 0 0.0
.text 576468 576468 0 0.0
linux chip-tool-ipv6only arm64 (read only) 8038388 8038420 32 0.0
(read/write) 370497 370497 0 0.0
.bss 55217 55217 0 0.0
.data 1096 1096 0 0.0
.data.rel.ro 245760 245760 0 0.0
.dynamic 560 560 0 0.0
.got 64728 64728 0 0.0
.init 24 24 0 0.0
.init_array 168 168 0 0.0
.rodata 419196 419196 0 0.0
.text 6830532 6830564 32 0.0
thermostat-no-ble arm64 (read only) 2041932 2041964 32 0.0
(read/write) 145969 145969 0 0.0
.bss 65089 65089 0 0.0
.data 880 880 0 0.0
.data.rel.ro 73016 73016 0 0.0
.dynamic 560 560 0 0.0
.got 4048 4048 0 0.0
.init 24 24 0 0.0
.init_array 304 304 0 0.0
.rodata 129884 129884 0 0.0
.text 1697456 1697488 32 0.0
p6 all-clusters-app default (read/write) 2402288 2402288 0 0.0
.bss 117484 117484 0 0.0
.data 2592 2592 0 0.0
.text 1360552 1360552 0 0.0
light-app default (read/write) 2327512 2327512 0 0.0
.bss 106064 106064 0 0.0
.data 2392 2392 0 0.0
.text 1285776 1285776 0 0.0
lock-app default (read/write) 2298424 2298424 0 0.0
.bss 104920 104920 0 0.0
.data 2344 2344 0 0.0
.text 1256688 1256688 0 0.0
qpg lighting-app qpg6105+debug (read only) 561944 561944 0 0.0
(read/write) 146936 146936 0 0.0
.bss 89952 89952 0 0.0
.data 1044 1044 0 0.0
.text 556624 556624 0 0.0
lock-app qpg6105+debug (read only) 515260 515260 0 0.0
(read/write) 146936 146936 0 0.0
.bss 88584 88584 0 0.0
.data 972 972 0 0.0
.text 509940 509940 0 0.0
persistent-storage-app qpg6105+debug (read only) 106848 106848 0 0.0
(read/write) 146940 146940 0 0.0
.bss 38512 38512 0 0.0
.data 288 288 0 0.0
.text 101528 101528 0 0.0
telink lighting-app tlsr9518adk80d (read/write) 838430 838406 -24 -0.0
bss 87312 87312 0 0.0
noinit 37160 37160 0 0.0
text 585584 585562 -22 -0.0

@bzbarsky-apple bzbarsky-apple merged commit 8aca71c into project-chip:master Jan 14, 2022
selissia pushed a commit to selissia/connectedhomeip that referenced this pull request Jan 28, 2022
- SessionManager has symetrical processing steps for
  group and unsecured message, but some side-effects of
  unicast secure sessions are delegated to SecureMessageCodec
  that should actually just be encrypt/decrypt logic.
- This is needed to assist in having a single point of tracing
  for incoming/outgoing messages (upcoming PR). Current organization
  requires putting that logic in several modules due to the
  side-effects occuring at the wrong layer.

Done by this PR:
- Hoists those side effects (e.g. updating counter) up from
  SecureMessageCodec where they do not belong, into SessionManager.
- Fix documentation and argument names of SecureMessageCodec that
  had rotted over many refactors and terminological changes.
- Add a bit of processing symmetry in SessionManager

Testing done: unit tests pass, integration tests pass
step0035 pushed a commit to hank820/connectedhomeip that referenced this pull request Feb 8, 2022
- SessionManager has symetrical processing steps for
  group and unsecured message, but some side-effects of
  unicast secure sessions are delegated to SecureMessageCodec
  that should actually just be encrypt/decrypt logic.
- This is needed to assist in having a single point of tracing
  for incoming/outgoing messages (upcoming PR). Current organization
  requires putting that logic in several modules due to the
  side-effects occuring at the wrong layer.

Done by this PR:
- Hoists those side effects (e.g. updating counter) up from
  SecureMessageCodec where they do not belong, into SessionManager.
- Fix documentation and argument names of SecureMessageCodec that
  had rotted over many refactors and terminological changes.
- Add a bit of processing symmetry in SessionManager

Testing done: unit tests pass, integration tests pass
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants