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.

Sign up for GitHub

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

Jump to bottom

Don't Clear mSecureSessionType on PairingSession::Clear() #13772

Merged
merged 4 commits into from
Jan 25, 2022
Merged

Don't Clear mSecureSessionType on PairingSession::Clear() #13772

merged 4 commits into from
Jan 25, 2022

Conversation

emargolis
Copy link
Contributor

Problem

#13711

Change overview

There is no need to clear the session type because it doesn't change.

I believe the more appropriate fix will be implemented with #9059. The Clear() method is not needed and a new session instance should be allocated and the old one released when required.

Testing

existing tests

@restyled-io restyled-io bot mentioned this pull request Jan 20, 2022
Closed
@emargolis emargolis mentioned this pull request Jan 20, 2022
Closed
@emargolis emargolis force-pushed the emargolis/feature/dont_clear_session_type branch from 8f4877e to 15e5ea3 Compare January 20, 2022 17:51
mlepage-google
mlepage-google approved these changes Jan 20, 2022
View reviewed changes
Copy link
Contributor

@mlepage-google mlepage-google left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do think this change should fix the issue, but please see the comment for a suggested tweak.

@github-actions
Copy link

github-actions bot commented Jan 20, 2022 •
edited
Loading

PR #13772: Size comparison from fa0b36f to 15e5ea3

Decreases (3 builds for efr32, p6, telink)
platform target config section fa0b36f 15e5ea3 change % change
efr32 lighting-app BRD4161A (read only) 832748 832732 -16 -0.0
.text 832740 832724 -16 -0.0
p6 light-app default (read/write) 2327832 2327816 -16 -0.0
.text 1286096 1286080 -16 -0.0
telink lighting-app tlsr9518adk80d (read/write) 839398 839390 -8 -0.0
text 586504 586496 -8 -0.0
Full report (14 builds for efr32, k32w, linux, p6, qpg, telink)
platform target config section fa0b36f 15e5ea3 change % change
efr32 lighting-app BRD4161A (read only) 832748 832732 -16 -0.0
(read/write) 127032 127032 0 0.0
.bss 125136 125136 0 0.0
.data 1896 1896 0 0.0
.text 832740 832724 -16 -0.0
BRD4161A+rpc (read only) 820128 820128 0 0.0
(read/write) 143696 143696 0 0.0
.bss 141696 141696 0 0.0
.data 1996 1996 0 0.0
.text 820120 820120 0 0.0
window-app BRD4161A (read only) 803316 803316 0 0.0
(read/write) 125720 125720 0 0.0
.bss 123872 123872 0 0.0
.data 1848 1848 0 0.0
.text 803308 803308 0 0.0
k32w light k32w061+release (read/write) 658572 658572 0 0.0
.bss 76584 76584 0 0.0
.data 1864 1864 0 0.0
.text 574324 574324 0 0.0
lock k32w061+release (read/write) 659368 659368 0 0.0
.bss 76824 76824 0 0.0
.data 1884 1884 0 0.0
.text 574860 574860 0 0.0
linux chip-tool-ipv6only arm64 (read only) 8377644 8377644 0 0.0
(read/write) 386113 386113 0 0.0
.bss 56049 56049 0 0.0
.data 1128 1128 0 0.0
.data.rel.ro 257200 257200 0 0.0
.dynamic 560 560 0 0.0
.got 67992 67992 0 0.0
.init 24 24 0 0.0
.init_array 200 200 0 0.0
.rodata 429972 429972 0 0.0
.text 7119508 7119508 0 0.0
thermostat-no-ble arm64 (read only) 2040876 2040876 0 0.0
(read/write) 145393 145393 0 0.0
.bss 64753 64753 0 0.0
.data 904 904 0 0.0
.data.rel.ro 72728 72728 0 0.0
.dynamic 560 560 0 0.0
.got 4064 4064 0 0.0
.init 24 24 0 0.0
.init_array 312 312 0 0.0
.rodata 130028 130028 0 0.0
.text 1696688 1696688 0 0.0
p6 all-clusters-app default (read/write) 2406600 2406600 0 0.0
.bss 117764 117764 0 0.0
.data 2576 2576 0 0.0
.text 1364864 1364864 0 0.0
light-app default (read/write) 2327832 2327816 -16 -0.0
.bss 105520 105520 0 0.0
.data 2408 2408 0 0.0
.text 1286096 1286080 -16 -0.0
lock-app default (read/write) 2296840 2296840 0 0.0
.bss 104368 104368 0 0.0
.data 2352 2352 0 0.0
.text 1255104 1255104 0 0.0
qpg lighting-app qpg6105+debug (read only) 565376 565376 0 0.0
(read/write) 146936 146936 0 0.0
.bss 89672 89672 0 0.0
.data 1060 1060 0 0.0
.text 560056 560056 0 0.0
lock-app qpg6105+debug (read only) 513868 513868 0 0.0
(read/write) 146940 146940 0 0.0
.bss 88240 88240 0 0.0
.data 984 984 0 0.0
.text 508548 508548 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) 839398 839390 -8 -0.0
bss 87468 87468 0 0.0
noinit 37160 37160 0 0.0
text 586504 586496 -8 -0.0

@github-actions
Copy link

github-actions bot commented Jan 20, 2022 •
edited
Loading

PR #13772: Size comparison from fa0b36f to ecb7832

Increases (24 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
platform target config section fa0b36f ecb7832 change % change
efr32 lighting-app BRD4161A (read only) 832748 833612 864 0.1
.text 832740 833604 864 0.1
BRD4161A+rpc (read only) 820128 821008 880 0.1
.text 820120 821000 880 0.1
window-app BRD4161A (read only) 803316 804196 880 0.1
.text 803308 804188 880 0.1
esp32 all-clusters-app c3devkit (read only) 916434 916846 412 0.0
(read/write) 1317506 1317522 16 0.0
.flash.rodata 178952 178968 16 0.0
.flash.text 916434 916846 412 0.0
m5stack (read only) 965003 965459 456 0.0
(read/write) 449464 449480 16 0.0
.flash.rodata 208120 208136 16 0.0
.flash.text 959619 960075 456 0.0
k32w light k32w061+release (read/write) 658572 658924 352 0.1
.text 574324 574676 352 0.1
lock k32w061+release (read/write) 659368 659704 336 0.1
.text 574860 575196 336 0.1
linux chip-tool-ipv6only arm64 (read only) 8377644 8390636 12992 0.2
(read/write) 386113 386417 304 0.1
.data.rel.ro 257200 257432 232 0.1
.got 67992 68056 64 0.1
.rodata 429972 430228 256 0.1
.text 7119508 7131604 12096 0.2
thermostat-no-ble arm64 (read only) 2040876 2043324 2448 0.1
(read/write) 145393 145425 32 0.0
.data.rel.ro 72728 72752 24 0.0
.text 1696688 1699136 2448 0.1
mbed all-clusters-app CY8CPROTO_062_4343W+release (read/write) 2349632 2349968 336 0.0
.text 1312208 1312544 336 0.0
lighting-app CY8CPROTO_062_4343W+release (read/write) 2332328 2332728 400 0.0
.text 1294928 1295328 400 0.0
lock-app CY8CPROTO_062_4343W+release (read/write) 2302744 2303080 336 0.0
.text 1265344 1265680 336 0.0
nrfconnect lighting-app nrf52840dk_nrf52840 (read/write) 940935 941287 352 0.0
rodata 108492 108508 16 0.0
text 635832 636164 332 0.1
nrf52840dk_nrf52840+rpc (read/write) 926415 926751 336 0.0
rodata 100940 100956 16 0.0
text 631228 631560 332 0.1
nrf52840dongle_nrf52840 (read/write) 991611 991963 352 0.0
rodata 113244 113260 16 0.0
text 668032 668364 332 0.0
nrf5340dk_nrf5340_cpuapp (read/write) 850762 851114 352 0.0
rodata 101668 101684 16 0.0
text 552756 553088 332 0.1
lock-app nrf52840dk_nrf52840 (read/write) 910295 910631 336 0.0
rodata 103460 103476 16 0.0
text 611280 611612 332 0.1
nrf5340dk_nrf5340_cpuapp (read/write) 820346 820698 352 0.0
rodata 96684 96700 16 0.0
text 528236 528568 332 0.1
pump-app nrf52840dk_nrf52840 (read/write) 913151 913503 352 0.0
rodata 103820 103836 16 0.0
text 613928 614260 332 0.1
pump-controller-app nrf52840dk_nrf52840 (read/write) 908231 908583 352 0.0
rodata 102932 102948 16 0.0
text 609856 610188 332 0.1
p6 all-clusters-app default (read/write) 2406600 2407464 864 0.0
.text 1364864 1365728 864 0.1
light-app default (read/write) 2327832 2328680 848 0.0
.text 1286096 1286944 848 0.1
lock-app default (read/write) 2296840 2297688 848 0.0
.text 1255104 1255952 848 0.1
telink lighting-app tlsr9518adk80d (read/write) 839398 839874 476 0.1
text 586504 586954 450 0.1
Full report (29 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
platform target config section fa0b36f ecb7832 change % change
efr32 lighting-app BRD4161A (read only) 832748 833612 864 0.1
(read/write) 127032 127032 0 0.0
.bss 125136 125136 0 0.0
.data 1896 1896 0 0.0
.text 832740 833604 864 0.1
BRD4161A+rpc (read only) 820128 821008 880 0.1
(read/write) 143696 143696 0 0.0
.bss 141696 141696 0 0.0
.data 1996 1996 0 0.0
.text 820120 821000 880 0.1
window-app BRD4161A (read only) 803316 804196 880 0.1
(read/write) 125720 125720 0 0.0
.bss 123872 123872 0 0.0
.data 1848 1848 0 0.0
.text 803308 804188 880 0.1
esp32 all-clusters-app c3devkit (read only) 916434 916846 412 0.0
(read/write) 1317506 1317522 16 0.0
.dram0.bss 70720 70720 0 0.0
.dram0.data 14244 14244 0 0.0
.flash.rodata 178952 178968 16 0.0
.flash.text 916434 916846 412 0.0
.iram0.text 62056 62056 0 0.0
m5stack (read only) 965003 965459 456 0.0
(read/write) 449464 449480 16 0.0
.dram0.bss 75184 75184 0 0.0
.dram0.data 34032 34032 0 0.0
.flash.rodata 208120 208136 16 0.0
.flash.text 959619 960075 456 0.0
.iram0.text 123399 123399 0 0.0
k32w light k32w061+release (read/write) 658572 658924 352 0.1
.bss 76584 76584 0 0.0
.data 1864 1864 0 0.0
.text 574324 574676 352 0.1
lock k32w061+release (read/write) 659368 659704 336 0.1
.bss 76824 76824 0 0.0
.data 1884 1884 0 0.0
.text 574860 575196 336 0.1
linux chip-tool-ipv6only arm64 (read only) 8377644 8390636 12992 0.2
(read/write) 386113 386417 304 0.1
.bss 56049 56049 0 0.0
.data 1128 1128 0 0.0
.data.rel.ro 257200 257432 232 0.1
.dynamic 560 560 0 0.0
.got 67992 68056 64 0.1
.init 24 24 0 0.0
.init_array 200 200 0 0.0
.rodata 429972 430228 256 0.1
.text 7119508 7131604 12096 0.2
thermostat-no-ble arm64 (read only) 2040876 2043324 2448 0.1
(read/write) 145393 145425 32 0.0
.bss 64753 64753 0 0.0
.data 904 904 0 0.0
.data.rel.ro 72728 72752 24 0.0
.dynamic 560 560 0 0.0
.got 4064 4064 0 0.0
.init 24 24 0 0.0
.init_array 312 312 0 0.0
.rodata 130028 130028 0 0.0
.text 1696688 1699136 2448 0.1
mbed all-clusters-app CY8CPROTO_062_4343W+release (read only) 6224 6224 0 0.0
(read/write) 2349632 2349968 336 0.0
.bss 189428 189428 0 0.0
.data 5296 5296 0 0.0
.text 1312208 1312544 336 0.0
lighting-app CY8CPROTO_062_4343W+release (read only) 6224 6224 0 0.0
(read/write) 2332328 2332728 400 0.0
.bss 180936 180936 0 0.0
.data 5576 5576 0 0.0
.text 1294928 1295328 400 0.0
lock-app CY8CPROTO_062_4343W+release (read only) 6224 6224 0 0.0
(read/write) 2302744 2303080 336 0.0
.bss 179936 179936 0 0.0
.data 5560 5560 0 0.0
.text 1265344 1265680 336 0.0
pigweed-app CY8CPROTO_062_4343W+release (read only) 6224 6224 0 0.0
(read/write) 1139712 1139712 0 0.0
.bss 11756 11756 0 0.0
.data 4368 4368 0 0.0
.text 103096 103096 0 0.0
shell CY8CPROTO_062_4343W+release (read only) 6224 6224 0 0.0
(read/write) 2054256 2054256 0 0.0
.bss 156876 156876 0 0.0
.data 4864 4864 0 0.0
.text 1016856 1016856 0 0.0
nrfconnect lighting-app nrf52840dk_nrf52840 (read/write) 940935 941287 352 0.0
bss 119020 119020 0 0.0
rodata 108492 108508 16 0.0
text 635832 636164 332 0.1
nrf52840dk_nrf52840+rpc (read/write) 926415 926751 336 0.0
bss 116064 116064 0 0.0
rodata 100940 100956 16 0.0
text 631228 631560 332 0.1
nrf52840dongle_nrf52840 (read/write) 991611 991963 352 0.0
bss 121864 121864 0 0.0
rodata 113244 113260 16 0.0
text 668032 668364 332 0.0
nrf5340dk_nrf5340_cpuapp (read/write) 850762 851114 352 0.0
bss 115808 115808 0 0.0
rodata 101668 101684 16 0.0
text 552756 553088 332 0.1
lock-app nrf52840dk_nrf52840 (read/write) 910295 910631 336 0.0
bss 118176 118176 0 0.0
rodata 103460 103476 16 0.0
text 611280 611612 332 0.1
nrf5340dk_nrf5340_cpuapp (read/write) 820346 820698 352 0.0
bss 114992 114992 0 0.0
rodata 96684 96700 16 0.0
text 528236 528568 332 0.1
pigweed-app nrf52840dk_nrf52840 (read/write) 541835 541835 0 0.0
bss 52588 52588 0 0.0
rodata 50104 50104 0 0.0
text 376940 376940 0 0.0
pump-app nrf52840dk_nrf52840 (read/write) 913151 913503 352 0.0
bss 117940 117940 0 0.0
rodata 103820 103836 16 0.0
text 613928 614260 332 0.1
pump-controller-app nrf52840dk_nrf52840 (read/write) 908231 908583 352 0.0
bss 117964 117964 0 0.0
rodata 102932 102948 16 0.0
text 609856 610188 332 0.1
shell nrf52840dk_nrf52840 (read/write) 798479 798479 0 0.0
bss 109776 109776 0 0.0
rodata 78324 78324 0 0.0
text 533872 533872 0 0.0
nrf5340dk_nrf5340_cpuapp (read/write) 711278 711278 0 0.0
bss 107664 107664 0 0.0
rodata 72624 72624 0 0.0
text 451548 451548 0 0.0
p6 all-clusters-app default (read/write) 2406600 2407464 864 0.0
.bss 117764 117764 0 0.0
.data 2576 2576 0 0.0
.text 1364864 1365728 864 0.1
light-app default (read/write) 2327832 2328680 848 0.0
.bss 105520 105520 0 0.0
.data 2408 2408 0 0.0
.text 1286096 1286944 848 0.1
lock-app default (read/write) 2296840 2297688 848 0.0
.bss 104368 104368 0 0.0
.data 2352 2352 0 0.0
.text 1255104 1255952 848 0.1
telink lighting-app tlsr9518adk80d (read/write) 839398 839874 476 0.1
bss 87468 87468 0 0.0
noinit 37160 37160 0 0.0
text 586504 586954 450 0.1

@woody-apple woody-apple linked an issue Jan 21, 2022 that may be closed by this pull request
PairingSession::GetSessionType returns incorrect result #13711
Closed
@andy31415
Copy link
Contributor

fast track: small delta, several checkmars, PR has been up for sufficient time for cross timezone review.

@andy31415 andy31415 merged commit d0cbf83 into project-chip:master Jan 25, 2022
selissia pushed a commit to selissia/connectedhomeip that referenced this pull request Jan 28, 2022
@emargolis @mlepage-google @selissia
Don't Clear mSecureSessionType on PairingSession::Clear() (project-ch…
ebe999a 
…ip#13772)

* Don't Clear mSecureSessionType on PairingSession::Clear()

* Update src/transport/PairingSession.h

Co-authored-by: Marc Lepage <[email protected]>

* Made mSecureSessionType const and initialize it from an arg to the constructor.

* cleanup

Co-authored-by: Marc Lepage <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mlepage-google mlepage-google mlepage-google approved these changes

@bzbarsky-apple bzbarsky-apple bzbarsky-apple approved these changes

@andy31415 andy31415 andy31415 approved these changes

@anush-apple anush-apple Awaiting requested review from anush-apple

@austinh0 austinh0 Awaiting requested review from austinh0

@balducci-apple balducci-apple Awaiting requested review from balducci-apple

@Byungjoo-Lee Byungjoo-Lee Awaiting requested review from Byungjoo-Lee

@carol-apple carol-apple Awaiting requested review from carol-apple

@chrisdecenzo chrisdecenzo Awaiting requested review from chrisdecenzo

@chshu chshu Awaiting requested review from chshu

@chulspro chulspro Awaiting requested review from chulspro

@Damian-Nordic Damian-Nordic Awaiting requested review from Damian-Nordic

@dhrishi dhrishi Awaiting requested review from dhrishi

@electrocucaracha electrocucaracha Awaiting requested review from electrocucaracha

@erjiaqing erjiaqing Awaiting requested review from erjiaqing

@franck-apple franck-apple Awaiting requested review from franck-apple

@gjc13 gjc13 Awaiting requested review from gjc13

@harimau-qirex harimau-qirex Awaiting requested review from harimau-qirex

@hawk248 hawk248 Awaiting requested review from hawk248

@holbrookt holbrookt Awaiting requested review from holbrookt

@jelderton jelderton Awaiting requested review from jelderton

@jepenven-silabs jepenven-silabs Awaiting requested review from jepenven-silabs

@jmartinez-silabs jmartinez-silabs Awaiting requested review from jmartinez-silabs

@LuDuda LuDuda Awaiting requested review from LuDuda

@msandstedt msandstedt Awaiting requested review from msandstedt

@mspang mspang Awaiting requested review from mspang

@pan-apple pan-apple Awaiting requested review from pan-apple

@robszewczyk robszewczyk Awaiting requested review from robszewczyk

@sagar-apple sagar-apple Awaiting requested review from sagar-apple

@saurabhst saurabhst Awaiting requested review from saurabhst

@selissia selissia Awaiting requested review from selissia

@tcarmelveilleux tcarmelveilleux Awaiting requested review from tcarmelveilleux

@tecimovic tecimovic Awaiting requested review from tecimovic

@vijs vijs Awaiting requested review from vijs

@vivien-apple vivien-apple Awaiting requested review from vivien-apple

@wbschiller wbschiller Awaiting requested review from wbschiller

@woody-apple woody-apple Awaiting requested review from woody-apple

@xylophone21 xylophone21 Awaiting requested review from xylophone21

@yufengwangca yufengwangca Awaiting requested review from yufengwangca

@yunhanw-google yunhanw-google Awaiting requested review from yunhanw-google

Assignees
No one assigned
Labels
fast track review - pending transport
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

PairingSession::GetSessionType returns incorrect result
4 participants
@emargolis @andy31415 @bzbarsky-apple @mlepage-google