Bluetooth: SDP: Add bt_sdp_unregister_service() API by chengkai15 · Pull Request #109101 · zephyrproject-rtos/zephyr

chengkai15 · 2026-05-14T05:32:58Z

Add bt_sdp_unregister_service() to complement the existing bt_sdp_register_service(). Currently there is no way to remove a Service Record from the SDP database once registered, which forces callers into workarounds (-EEXIST tolerance, guard flags that can never be reset, no-op unregister stubs).

The implementation follows the same pattern as bt_l2cap_br_server_unregister(): NULL check → sys_slist_find_and_remove() → LOG_DBG → return.

lylezhu2012

Why is the file tests/bluetooth/classic/sdp_s/src/sdp_server.c changed in this PR?

lylezhu2012 · 2026-05-18T08:22:01Z

I think the SDP service cannot be removed anytime.
At least, No SDP discovery is ongoing. Even, it needs to ensure that there are no dynamic L2CAP connections. Even more securely, There is no ACL connections.

And I think we need to add more comments to explain the potential risks associated with this change.

Thanks for the review @lylezhu2012, this is a valid concern.

I agree that blindly removing a service record from sdp_db is unsafe if there are active SDP transactions (especially multi-PDU responses with continuation state). Here are a few options maybe for adding safety:

Documentation only — Add detailed comments/doxygen explaining the preconditions (no active SDP transactions, ideally no ACL connections), leave enforcement to the caller. This matches how
bt_l2cap_br_server_unregister() works today.

Runtime check on SDP L2CAP channel — Return -EBUSY if any SDP PSM (0x0001) L2CAP channel is currently connected. This catches the most obvious dangerous case.

Runtime check on BR/EDR ACL connections — Return -EBUSY if any BT_CONN_TYPE_BR connection exists. Safest, but may be too restrictive for use cases that need dynamic service registration/unregistration
while connected.

Hybrid (option 2 + documentation) — Check for active SDP L2CAP channels at runtime, and document the residual race window (a remote device could open an SDP channel between the check and the removal) as
the caller's responsibility.

it seem option 4 is a good chocie . I'd like to hear what approach other's idea before making changes.

@lylezhu2012 @jhedberg What do you think?

I've done with option 4 . any feadback?

Copilot

Pull request overview

This PR adds a missing Classic Bluetooth SDP server API to unregister (remove) a previously registered SDP Service Record, complementing the existing bt_sdp_register_service() and reducing the need for caller-side workarounds.

Changes:

Add public API + Doxygen documentation for bt_sdp_unregister_service().
Implement bt_sdp_unregister_service() in the SDP server, including an “active SDP channel” -EBUSY guard.
Extend the classic SDP server test shell to support unregistering a record by index.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File	Description
subsys/bluetooth/host/classic/sdp.c	Implements `bt_sdp_unregister_service()` by removing a record from the internal SDP DB, with a busy check for active SDP server channels.
include/zephyr/bluetooth/classic/sdp.h	Declares the new public API and documents expected return values and concurrency caveats.
tests/bluetooth/classic/sdp_s/src/sdp_server.c	Adds a shell command to unregister a test SDP record, updating the test’s local “registered” tracking.

chengkai15 · 2026-05-19T13:19:02Z

+	if (!sys_slist_find_and_remove(&sdp_db, &service->node)) {
+		return -ENOENT;
+	}
+
+	LOG_DBG("Service unregistered at %u", service->handle);
+
+	return 0;


remove redundant num_services

chengkai15 · 2026-05-19T13:20:16Z

+	for (int i = 0; i < ARRAY_SIZE(bt_sdp_pool); i++) {
+		if (bt_sdp_pool[i].chan.chan.conn) {
+			LOG_WRN("Active SDP channel exists on conn %p",
+				bt_sdp_pool[i].chan.chan.conn);
+			return -EBUSY;
+		}
+	}


chengkai15 · 2026-05-19T13:25:12Z

+	index = strtoul(argv[1], NULL, 16);
+	if (index >= MAX_SDP_RECORD_COUNT) {
+		shell_error(sh, "Invalid index %d", index);
+		return -EINVAL;
+	}


chengkai15 · 2026-05-19T13:27:18Z

+
+	if (!sdp_rec_reg[index]) {
+		shell_error(sh, "The SDP record %d is not registered", index);
+		return -ENOEXEC;


-ENOEXEC was Zephyr shell command stand pattern.which was not a need

lylezhu2012 · 2026-05-19T10:24:23Z

+		return -EINVAL;
+	}
+
+	for (int i = 0; i < ARRAY_SIZE(bt_sdp_pool); i++) {


Suggested change

for (int i = 0; i < ARRAY_SIZE(bt_sdp_pool); i++) {

ARRAY_FOR_EACH(bt_sdp_pool, i) {

lylezhu2012 · 2026-05-19T10:28:55Z

+			return -EBUSY;
+		}
+	}
+


I think the SDP session connections of SDP Client should also be checked.

thanks for your suggestion

The bt_sdp_unregister_service() only modifies the local SDP server database (sdp_db). The SDP client pool (bt_sdp_client_pool) holds channels used to discover remote devices' SDP databases — those channels
never access or iterate over our local sdp_db.

Therefore, an active outgoing SDP client session has no interaction with the local service record being removed, and checking it would be unnecessary

lylezhu2012 · 2026-05-19T10:29:12Z

+	}
+
+	for (int i = 0; i < ARRAY_SIZE(bt_sdp_pool); i++) {
+		if (bt_sdp_pool[i].chan.chan.conn) {


Suggested change

if (bt_sdp_pool[i].chan.chan.conn) {

if (bt_sdp_pool[i].chan.chan.conn != NULL) {

lylezhu2012 · 2026-05-19T10:32:52Z

Why do you want to add the shell commander to this file? This is a test suite for SDP server role. If you want to add a shell commander for the changes, I think the test case is needed for this shell commander.

@lylezhu2012 thanks for your suggest

The unregister_sdp shell command is placed in sdp_server.c because it operates on the local SDP server database (sdp_db) — the same database that register_sdp and register_sdp_all manage in this file. It is the inverse of register_sdp, so co-locating them is natural.

I've also added a pytest test case (test_sdp_unregister) that exercises the full lifecycle: register → verify discoverable → disconnect SDP → unregister → reconnect → verify gone → re-register → verify discoverable again. This validates both the shell command and the -EBUSY guard requiring SDP disconnection before removal.

sonarqubecloud · 2026-05-27T06:14:42Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

chengkai15 · 2026-06-03T02:14:27Z

@lylezhu2012 @jhedberg would appreciate another look when you have a moment

lylezhu2012 · 2026-06-30T03:47:22Z

Actually, the condition is not always true. The value of state.current_svc is came from the sdp require of peer device. This is my concern about this change.

I hope to have CI/CV to ensure that changes are error-free. So this PR is also the motivation for my current work on classic simulation.

The num_services counter was only used in a single condition (state.current_svc < num_services) within the SSA response continuation state logic. Since record indices are assigned sequentially from 0 and state.current_svc is always set from record->index (which is always < num_services for any valid record), this condition is always true — making it dead code.

Remove the variable, its increment in bt_sdp_register_service(), and the redundant condition wrapper, unconditionally entering dry_run mode when the packet is full. dc5f28f

lylezhu2012 · 2026-06-30T03:48:23Z

Did you run the test suites locally? I cannot pass the test suite.

I test pass a month ago, I am trying it agin

Thanks for flagging this — you were right, the suite did not pass locally. I tracked it down and fixed it; 4/4 cases pass now.

Root cause was in the test shell command, not the SDP code:

cmd_unregister_sdp() declared int err; (uninitialized) and then called shell_strtoul(argv[1], 16, &err). Unlike strtoul(), shell_strtoul() only writes *err on the error paths — on the success path it returns directly (subsys/shell/shell_utils.c:661) without touching *err. So err kept a stale stack value, the if (err || ...) guard tripped, and the command printed Invalid index 0 and returned without ever calling bt_sdp_unregister_service().

That made test_sdp_unregister fail at the "record is gone" assertion, and because the DUT is session-scoped the leftover record then polluted test_sdp_discover, which asserts an empty database at the start.

Fix: initialize int err = 0; before the shell_strtoul() call (amended into the test commit).

Two more things were needed to actually run the suite on native_sim (these are infrastructure gaps, not SDP bugs):

CONFIG_ZTEST_SHELL=y in prj.conf — otherwise the native_sim image runs an empty ztest and exits before the pytest harness can attach to the shell.

CONFIG_UART_NATIVE_PTY_0_ON_STDINOUT=y via testcase.yaml extra_args — so the shell UART is on stdin/stdout (the harness uses process pipes, not a PTY).

With those + the err fix, the full lifecycle passes: register → discoverable → disconnect → unregister → gone → re-register → discoverable again.

Re state.current_svc (sdp.c:1521): test_sdp_discover_with_range registers the large record and does a full-attribute scan, which exercises multi-PDU SSA continuation across many rounds, and it passes — so removing the if (state.current_svc < num_services) guard does not break the continuation path for the cases covered here. I agree the peer-controlled continuation value is worth broader CV coverage, which I understand is what your classic simulation work targets.

lylezhu2012 · 2026-07-03T09:58:06Z

+	}
+
+	err = bt_sdp_unregister_service(&spp_rec[index]);
+	if (err) {


Suggested change

if (err) {

if (err != 0) {

sonarqubecloud · 2026-07-09T12:44:48Z

❌ The last analysis has failed.

See analysis details on SonarQube Cloud

github-actions · 2026-08-14T00:41:19Z

This pull request has been marked as stale because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 7 days. Note that if it gets closed, you can ask someone to reopen it for you if you do not have the permissions to do so.

The num_services counter was only used in a single condition (state.current_svc < num_services) within the SSA response continuation state logic. Since record indices are assigned sequentially from 0 and state.current_svc is always set from record->index (which is always < num_services for any valid record), this condition is always true — making it dead code. Remove the variable, its increment in bt_sdp_register_service(), and the redundant condition wrapper, unconditionally entering dry_run mode when the packet is full. Signed-off-by: Kai Cheng <chengkai@xiaomi.com>

Add a new API to remove a previously registered Service Record from the SDP database. This complements bt_sdp_register_service() and follows the same pattern as bt_l2cap_br_server_unregister(). The function checks for active SDP server L2CAP channels and returns -EBUSY if any exist, preventing unsafe removal during active transactions. A residual race window remains between the check and the actual removal; the caller is responsible for ensuring no new SDP connections are established during this call. Signed-off-by: Kai Cheng <chengkai@xiaomi.com>

…test Add a shell command to unregister SDP service records by index, and a pytest test case that verifies the full lifecycle: 1. Register a record and verify it is discoverable 2. Disconnect SDP, unregister, reconnect and verify it is gone 3. Re-register and verify it is discoverable again Initialize the error status variable before calling shell_strtoul(): unlike strtoul(), shell_strtoul() does not clear the output error code on the success path, so an uninitialized 'err' left a stale stack value that made the 'Invalid index' guard trip and silently skipped the actual unregister call. Also enable CONFIG_ZTEST_SHELL (so the native_sim image stays in the shell instead of exiting after an empty ztest run) and route the shell UART to stdin/stdout via CONFIG_UART_NATIVE_PTY_0_ON_STDINOUT, so the pytest twister harness can drive the shell over the process pipes. Signed-off-by: Kai Cheng <chengkai@xiaomi.com>

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

zephyrbot added area: Bluetooth area: Bluetooth Classic Bluetooth Classic (BR/EDR) area: Tests Issues related to a particular existing or missing test labels May 14, 2026

zephyrbot requested review from MarkWangChinese, gzh-terry, jhedberg, lylezhu2012, makeshi and nashif May 14, 2026 05:34

zephyrbot assigned lylezhu2012 May 14, 2026

chengkai15 force-pushed the sdp_unregister_service branch from cfb3cac to 7fd34cf Compare May 14, 2026 05:40

jhedberg previously approved these changes May 15, 2026

View reviewed changes

lylezhu2012 reviewed May 18, 2026

View reviewed changes

chengkai15 dismissed jhedberg’s stale review via 0408078 May 19, 2026 09:02

chengkai15 force-pushed the sdp_unregister_service branch from 7fd34cf to 0408078 Compare May 19, 2026 09:02

chengkai15 requested a review from Copilot May 19, 2026 09:02

Copilot started reviewing on behalf of chengkai15 May 19, 2026 09:02 View session

chengkai15 requested review from jhedberg and lylezhu2012 May 19, 2026 09:05

Copilot AI reviewed May 19, 2026

View reviewed changes

lylezhu2012 reviewed May 19, 2026

View reviewed changes

chengkai15 force-pushed the sdp_unregister_service branch from 0408078 to faea9ec Compare May 19, 2026 13:41

chengkai15 requested a review from lylezhu2012 May 19, 2026 13:43

chengkai15 force-pushed the sdp_unregister_service branch 2 times, most recently from 8082d46 to f350d55 Compare May 26, 2026 12:43

chengkai15 force-pushed the sdp_unregister_service branch from 29cd43d to f350d55 Compare May 27, 2026 05:13

jhedberg previously approved these changes Jun 3, 2026

View reviewed changes

lylezhu2012 reviewed Jun 30, 2026

View reviewed changes

chengkai15 dismissed jhedberg’s stale review via 219f299 June 30, 2026 13:12

chengkai15 force-pushed the sdp_unregister_service branch 2 times, most recently from 219f299 to e1b6c29 Compare June 30, 2026 13:20

chengkai15 requested review from jhedberg and lylezhu2012 July 2, 2026 12:56

lylezhu2012 reviewed Jul 3, 2026

View reviewed changes

chengkai15 force-pushed the sdp_unregister_service branch 2 times, most recently from 73e058d to 282a610 Compare July 9, 2026 12:21

chengkai15 requested a review from lylezhu2012 July 14, 2026 07:28

github-actions Bot added the Stale label Aug 14, 2026

github-actions Bot closed this Aug 22, 2026

chengkai15 reopened this Aug 26, 2026

chengkai15 added 3 commits August 26, 2026 10:41

chengkai15 force-pushed the sdp_unregister_service branch from 282a610 to 537b3c0 Compare August 26, 2026 02:49

chengkai15 requested a lite review from Copilot August 26, 2026 02:51

Copilot started reviewing on behalf of chengkai15 August 26, 2026 02:51 View session

Copilot AI reviewed Aug 26, 2026

View reviewed changes

	for (int i = 0; i < ARRAY_SIZE(bt_sdp_pool); i++) {
	ARRAY_FOR_EACH(bt_sdp_pool, i) {

	if (bt_sdp_pool[i].chan.chan.conn) {
	if (bt_sdp_pool[i].chan.chan.conn != NULL) {

Uh oh!

Conversation

chengkai15 commented May 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lylezhu2012 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chengkai15 May 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud Bot commented May 27, 2026

Quality Gate passed

Uh oh!

chengkai15 commented Jun 3, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud Bot commented Jul 9, 2026

Uh oh!

github-actions Bot commented Aug 14, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

chengkai15 commented May 14, 2026 •

edited

Loading

chengkai15 May 18, 2026 •

edited

Loading