[BlueZ,1/2] device: Add method GetServiceRecords

Commit Message

Vibhav Pant Sept. 7, 2024, 9:33 p.m. UTC

GetServiceRecords returns all currently known BR/EDR service records
for the device, as an array of array of bytes. Each individual byte
array represents a raw SDP record, as defined by the Bluetooth Service
Discovery Protocol spec.
This method is intended to be only used by compatibility layers like
Wine, that need to provide access to raw SDP records to implement the
Win32 Bluetooth API. Applications should instead use the Profile API
for services-related functionality.
---
 src/device.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 83 insertions(+)

Comments

[e-mail] Sept. 8, 2024, 12:04 a.m. UTC | #1

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=888047

---Test result---

Test Summary:
CheckPatch                    PASS      1.05 seconds
GitLint                       PASS      0.66 seconds
BuildEll                      PASS      24.72 seconds
BluezMake                     PASS      1682.04 seconds
MakeCheck                     PASS      13.55 seconds
MakeDistcheck                 PASS      178.67 seconds
CheckValgrind                 PASS      252.77 seconds
CheckSmatch                   PASS      356.35 seconds
bluezmakeextell               PASS      119.86 seconds
IncrementalBuild              PASS      2978.48 seconds
ScanBuild                     PASS      1001.94 seconds



---
Regards,
Linux Bluetooth

Luiz Augusto von Dentz Sept. 9, 2024, 3:16 p.m. UTC | #2

Hi Vibhav,

On Sat, Sep 7, 2024 at 5:33 PM Vibhav Pant <[e-mail]> wrote:
>
> GetServiceRecords returns all currently known BR/EDR service records
> for the device, as an array of array of bytes. Each individual byte
> array represents a raw SDP record, as defined by the Bluetooth Service
> Discovery Protocol spec.
> This method is intended to be only used by compatibility layers like
> Wine, that need to provide access to raw SDP records to implement the
> Win32 Bluetooth API. Applications should instead use the Profile API
> for services-related functionality.

Is this the best format though? I'm afraid this would require an upper
level to implement SDP record parsing as well or is that what Windows
exposes as well?

> ---
>  src/device.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 83 insertions(+)
>
> diff --git a/src/device.c b/src/device.c
> index 0f18c8c7f..6aa809c80 100644
> --- a/src/device.c
> +++ b/src/device.c
> @@ -3215,6 +3215,86 @@ static DBusMessage *cancel_pairing(DBusConnection *conn, DBusMessage *msg,
>         return dbus_message_new_method_return(msg);
>  }
>
> +static sdp_list_t *read_device_records(struct btd_device *device);
> +
> +static DBusMessage *get_service_records(DBusConnection *conn, DBusMessage *msg,
> +                                       void *data)
> +{
> +       DBusMessage *reply;
> +       DBusMessageIter records_arr, record;
> +       struct btd_device *device = data;
> +       sdp_list_t *cur;
> +
> +       if (!btd_adapter_get_powered(device->adapter))
> +               return btd_error_not_ready(msg);
> +
> +       if (!btd_device_is_connected(device))
> +               return btd_error_not_connected(msg);
> +
> +       if (!device->bredr_state.svc_resolved)
> +               return btd_error_not_ready(msg);
> +
> +       if (!device->tmp_records) {
> +               device->tmp_records = read_device_records(device);
> +               if (!device->tmp_records)
> +                       return btd_error_does_not_exist(msg);
> +       }
> +
> +       reply = dbus_message_new_method_return(msg);
> +       if (!reply)
> +               return btd_error_failed(msg, "Could not create method reply");
> +
> +       dbus_message_iter_init_append(reply, &records_arr);
> +       if (!dbus_message_iter_open_container(&records_arr, DBUS_TYPE_ARRAY,
> +                                             "ay", &record)) {
> +               dbus_message_unref(reply);
> +               return btd_error_failed(msg, "Could not initialize iterator");
> +       }
> +
> +       for (cur = device->tmp_records; cur; cur = cur->next) {
> +               DBusMessageIter record_bytes;
> +               sdp_record_t *rec = cur->data;
> +               sdp_buf_t buf;
> +               int result;
> +
> +               result = sdp_gen_record_pdu(rec, &buf);
> +               if (result) {
> +                       dbus_message_iter_abandon_container(&records_arr,
> +                                                           &record);
> +                       dbus_message_unref(reply);
> +                       return btd_error_failed(
> +                               msg, "Could not marshal service record");
> +               }
> +               if (!dbus_message_iter_open_container(&record, DBUS_TYPE_ARRAY,
> +                                                     "y", &record_bytes)) {
> +                       bt_free(buf.data);
> +                       dbus_message_iter_abandon_container(&records_arr,
> +                                                           &record);
> +                       dbus_message_unref(reply);
> +                       return btd_error_failed(
> +                               msg, "Could not initialize iterator");
> +               }
> +               if (!dbus_message_iter_append_fixed_array(
> +                           &record_bytes, DBUS_TYPE_BYTE, &buf.data,
> +                           buf.data_size)) {
> +                       bt_free(buf.data);
> +                       dbus_message_iter_abandon_container(&record,
> +                                                           &record_bytes);
> +                       dbus_message_iter_abandon_container(&records_arr,
> +                                                           &record);
> +                       dbus_message_unref(reply);
> +                       return btd_error_failed(
> +                               msg, "Could not append record data to reply");
> +               }
> +               dbus_message_iter_close_container(&record, &record_bytes);
> +               bt_free(buf.data);
> +       }
> +
> +       dbus_message_iter_close_container(&records_arr, &record);
> +
> +       return reply;
> +}
> +
>  static const GDBusMethodTable device_methods[] = {
>         { GDBUS_ASYNC_METHOD("Disconnect", NULL, NULL, dev_disconnect) },
>         { GDBUS_ASYNC_METHOD("Connect", NULL, NULL, dev_connect) },
> @@ -3224,6 +3304,9 @@ static const GDBusMethodTable device_methods[] = {
>                                                 NULL, disconnect_profile) },
>         { GDBUS_ASYNC_METHOD("Pair", NULL, NULL, pair_device) },
>         { GDBUS_METHOD("CancelPairing", NULL, NULL, cancel_pairing) },
> +       { GDBUS_EXPERIMENTAL_METHOD("GetServiceRecords", NULL,
> +                                   GDBUS_ARGS({ "Records", "aay" }),
> +                                   get_service_records) },
>         { }
>  };
>
> --
> 2.46.0
>
>

Vibhav Pant Sept. 9, 2024, 4:04 p.m. UTC | #3

Hi Luiz,

On Mon, Sep 9, 2024 at 8:47 PM Luiz Augusto von Dentz
<[e-mail]> wrote:
> Is this the best format though? I'm afraid this would require an upper
> level to implement SDP record parsing as well or is that what Windows
> exposes as well?

Yes, the service discovery related interfaces on Windows return a raw
SDP byte stream, which the user is expected to further parse with
methods like BluetoothSdpEnumAttributes[1]. That being said, my
primary motivation for returning the raw SDP data was to also
discourage other applications from using this instead of the Profile
API.
Essentially, the Windows kernel and usermode Bluetooth APIs let you
make SDP queries to Bluetooth devices (including service and service +
attribute queries). From my understanding, exposing raw SDP is an
antipattern in BlueZ that was discarded with BlueZ 5, so I did not
want to re-introduce the entire concept again over DBus. Instead, the
current design of the Wine bluetooth driver is to rely on the cached
records stored by BlueZ, and perform any and all queries by searching
through them.
The other option was to rely on sdp_lib.h, but having two SDP local
servers (is it a local server if it's only being used for device
queries?) for every machine sounds like a recipe for eventual
breakage, and letting BlueZ be the authoritative source of truth for
device-related information is a common pattern in Wine.

[1]: https://learn.microsoft.com/en-us/windows/win32/api/bluetoothapis/nf-bluetoothapis-bluetoothsdpenumattributes

Patch

diff --git a/src/device.c b/src/device.c
index 0f18c8c7f..6aa809c80 100644
--- a/src/device.c
+++ b/src/device.c
@@ -3215,6 +3215,86 @@  static DBusMessage *cancel_pairing(DBusConnection *conn, DBusMessage *msg,
 	return dbus_message_new_method_return(msg);
 }
 
+static sdp_list_t *read_device_records(struct btd_device *device);
+
+static DBusMessage *get_service_records(DBusConnection *conn, DBusMessage *msg,
+					void *data)
+{
+	DBusMessage *reply;
+	DBusMessageIter records_arr, record;
+	struct btd_device *device = data;
+	sdp_list_t *cur;
+
+	if (!btd_adapter_get_powered(device->adapter))
+		return btd_error_not_ready(msg);
+
+	if (!btd_device_is_connected(device))
+		return btd_error_not_connected(msg);
+
+	if (!device->bredr_state.svc_resolved)
+		return btd_error_not_ready(msg);
+
+	if (!device->tmp_records) {
+		device->tmp_records = read_device_records(device);
+		if (!device->tmp_records)
+			return btd_error_does_not_exist(msg);
+	}
+
+	reply = dbus_message_new_method_return(msg);
+	if (!reply)
+		return btd_error_failed(msg, "Could not create method reply");
+
+	dbus_message_iter_init_append(reply, &records_arr);
+	if (!dbus_message_iter_open_container(&records_arr, DBUS_TYPE_ARRAY,
+					      "ay", &record)) {
+		dbus_message_unref(reply);
+		return btd_error_failed(msg, "Could not initialize iterator");
+	}
+
+	for (cur = device->tmp_records; cur; cur = cur->next) {
+		DBusMessageIter record_bytes;
+		sdp_record_t *rec = cur->data;
+		sdp_buf_t buf;
+		int result;
+
+		result = sdp_gen_record_pdu(rec, &buf);
+		if (result) {
+			dbus_message_iter_abandon_container(&records_arr,
+							    &record);
+			dbus_message_unref(reply);
+			return btd_error_failed(
+				msg, "Could not marshal service record");
+		}
+		if (!dbus_message_iter_open_container(&record, DBUS_TYPE_ARRAY,
+						      "y", &record_bytes)) {
+			bt_free(buf.data);
+			dbus_message_iter_abandon_container(&records_arr,
+							    &record);
+			dbus_message_unref(reply);
+			return btd_error_failed(
+				msg, "Could not initialize iterator");
+		}
+		if (!dbus_message_iter_append_fixed_array(
+			    &record_bytes, DBUS_TYPE_BYTE, &buf.data,
+			    buf.data_size)) {
+			bt_free(buf.data);
+			dbus_message_iter_abandon_container(&record,
+							    &record_bytes);
+			dbus_message_iter_abandon_container(&records_arr,
+							    &record);
+			dbus_message_unref(reply);
+			return btd_error_failed(
+				msg, "Could not append record data to reply");
+		}
+		dbus_message_iter_close_container(&record, &record_bytes);
+		bt_free(buf.data);
+	}
+
+	dbus_message_iter_close_container(&records_arr, &record);
+
+	return reply;
+}
+
 static const GDBusMethodTable device_methods[] = {
 	{ GDBUS_ASYNC_METHOD("Disconnect", NULL, NULL, dev_disconnect) },
 	{ GDBUS_ASYNC_METHOD("Connect", NULL, NULL, dev_connect) },
@@ -3224,6 +3304,9 @@  static const GDBusMethodTable device_methods[] = {
 						NULL, disconnect_profile) },
 	{ GDBUS_ASYNC_METHOD("Pair", NULL, NULL, pair_device) },
 	{ GDBUS_METHOD("CancelPairing", NULL, NULL, cancel_pairing) },
+	{ GDBUS_EXPERIMENTAL_METHOD("GetServiceRecords", NULL,
+				    GDBUS_ARGS({ "Records", "aay" }),
+				    get_service_records) },
 	{ }
 };