Bluetooth: Host: Create forward declaration for guarded functions #82552
Conversation
The functions `le_sc_oob_config_set`, `generate_dhkey` and `display_passkey` in `smp.c` were only defined when `CONFIG_BT_SMP_OOB_LEGACY_PAIR_ONLY` is selected. This created issues at build time. Add a forward declation of the listed function outside of the guarding to fix the issue. Signed-off-by: Théo Battrel <[email protected]>
zephyrbot
requested review from
alwa-nordic,
hermabe,
jhedberg,
rugeGerritsen,
sjanc and
Thalley
The new `host_config_variants` goal is to be able to test specific set of configurations when building the Host without the need of having a specific application for it. The first test check that the Host build correctly with `CONFIG_BT_SMP_OOB_LEGACY_PAIR_ONLY` enabled. Signed-off-by: Théo Battrel <[email protected]>
theob-pro
force-pushed
the
bt-smp-oob-legacy-pair-only
branch
from
7f83918
to
764e9ff
Compare
| @@ -3685,6 +3685,10 @@ static void le_sc_oob_config_set(struct bt_smp *smp, | |||
|
|
|||
| info->lesc.oob_config = oob_config; | |||
| } | |||
| #else | |||
|
|
|||
| void le_sc_oob_config_set(struct bt_smp *smp, struct bt_conn_oob_info *info); | |||
There was a problem hiding this comment.
Missing static. However, why are not instead moving the function definition to the right place and behind the right build-time conditionals (which match the conditionals of its usage)?
There was a problem hiding this comment.
Missing
static.
We're getting the following warning when using static:
/home/thb1-local/zephyrproject/zephyr/subsys/bluetooth/host/smp.c:3690:13: error: ‘le_sc_oob_config_set’ used but never defined [-Werror]
3690 | static void le_sc_oob_config_set(struct bt_smp *smp, struct bt_conn_oob_info *info);
| ^~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
However, why are not instead moving the function definition to the right place and behind the right build-time conditionals (which match the conditionals of its usage)?
The use site doesn't have conditional compile. It relies on dead code elimination by the compiler to avoid a linker error.
Do you have a suggested solution?
There was a problem hiding this comment.
Move the definition out so that it's not restricted by any build time conditions?
There was a problem hiding this comment.
They have been added there: #74400
It seems they were needed
There was a problem hiding this comment.
I did this quick change on top of your PR, and it seems to work fine:
diff --git a/subsys/bluetooth/host/smp.c b/subsys/bluetooth/host/smp.c
index c737f59e958..62028827ebf 100644
--- a/subsys/bluetooth/host/smp.c
+++ b/subsys/bluetooth/host/smp.c
@@ -3640,7 +3640,6 @@ static uint8_t sc_smp_check_confirm(struct bt_smp *smp)
return 0;
}
-#ifndef CONFIG_BT_SMP_OOB_LEGACY_PAIR_ONLY
static bool le_sc_oob_data_req_check(struct bt_smp *smp)
{
struct bt_smp_pairing *req = (struct bt_smp_pairing *)&smp->preq[1];
@@ -3685,11 +3684,6 @@ static void le_sc_oob_config_set(struct bt_smp *smp,
info->lesc.oob_config = oob_config;
}
-#else
-
-void le_sc_oob_config_set(struct bt_smp *smp, struct bt_conn_oob_info *info);
-
-#endif /* CONFIG_BT_SMP_OOB_LEGACY_PAIR_ONLY */
static uint8_t smp_pairing_random(struct bt_smp *smp, struct net_buf *buf)
{
@@ -4190,7 +4184,6 @@ static uint8_t smp_security_request(struct bt_smp *smp, struct net_buf *buf)
}
#endif /* CONFIG_BT_CENTRAL */
-#ifndef CONFIG_BT_SMP_OOB_LEGACY_PAIR_ONLY
static uint8_t generate_dhkey(struct bt_smp *smp)
{
if (IS_ENABLED(CONFIG_BT_SMP_OOB_LEGACY_PAIR_ONLY)) {
@@ -4232,12 +4225,6 @@ static uint8_t display_passkey(struct bt_smp *smp)
return 0;
}
-#else
-
-uint8_t generate_dhkey(struct bt_smp *smp);
-uint8_t display_passkey(struct bt_smp *smp);
-
-#endif /* CONFIG_BT_SMP_OOB_LEGACY_PAIR_ONLY */
#if defined(CONFIG_BT_PERIPHERAL)
static uint8_t smp_public_key_periph(struct bt_smp *smp)There was a problem hiding this comment.
But do you understand why the guarding were added in first place? I am a bit afraid of breaking something by removing them...
Maybe @spanceac could have a look here?
There was a problem hiding this comment.
They have been added there: #74400 It seems they were needed
Seems like that PR was buggy, and the issue should have been solved in a different way. I wonder what configuration I should try with my proposed change above to verify that it's fine. It builds cleanly with your new test app, as well as with the default configuration for nrf52840dk with peripheral_hr and peripheral_sc_only.
There was a problem hiding this comment.
If it works without the ifdefs then we should go for that.
See commit messages for details.
Fix #82550