-
Notifications
You must be signed in to change notification settings - Fork 6.1k
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
tests: Bluetooth: shell: Fix connection reference leak #66484
tests: Bluetooth: shell: Fix connection reference leak #66484
Conversation
This fixes regression introduced in recently. Redundant connection reference is taken twice in connected() callback. The redunant reference was then not returned, and as a result we had a leak. Signed-off-by: Mariusz Skamra <mariusz.skamra@codecoup.pl>
This basically reverts changes made by 9c8ec58. Signed-off-by: Mariusz Skamra <mariusz.skamra@codecoup.pl>
81a3530
to
a43d9d4
Compare
err = bt_conn_le_create(&addr, create_params, BT_LE_CONN_PARAM_DEFAULT, | ||
&conn); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like it'd make sense to update bt_conn_le_create()
to accept NULL
as the last parameter in cases like this (i.e. where you don't do anything meaningful with the pointer even after a successful return).
@@ -694,8 +694,6 @@ static void connected(struct bt_conn *conn, uint8_t err) | |||
} | |||
} | |||
|
|||
default_conn = bt_conn_ref(conn); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see the bug with the ref getting taken twice for ROLE_PERIPHERAL
, but where is default_conn
getting set for ROLE_CENTRAL
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see the bug with the ref getting taken twice for
ROLE_PERIPHERAL
, but where isdefault_conn
getting set forROLE_CENTRAL
?
Nevermind, I'm just blind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MariuszSkamra, this seems to fix the double reference problem. Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also it fixes re-advertising after a disconnection.
This fixes regression introduced in recently.
Redundant connection reference is taken twice in connected()
callback. The redunant reference was then not returned, and
as a result we had a leak.