[PATCH BlueZ 0/3] Prerequisites for playstation-peripheral _external_ plugin

[PATCH BlueZ 0/3] Prerequisites for playstation-peripheral _external_ plugin

[Antonio Ospite]

Hi,

I proposed these changes back in September[1] but they didn't receive
a lot of comments, I know they are not very interesting to most of you,
but let me repeat the rationale behind them.

  1. It was decided to have the playstation-peripheral as an external
     plugin because it is going to be used by few people and it is not
     worth to keep it in the bluetoothd binary. Distributors could
     choose to put external plugins in their own packages so to provide
     a install-time choice instead of a compile-time choice to use the
     plugin services.

  2. external plugins can only access symbols exported as global in
     src/bluetooth.ver, like btd_* and very few others.

  3. It was suggested to add some helper functions to the bluez code
     instead of adding new global symbols to bluetooth.ver

Some notes about the implementation: I choose to pass only strings in
function signatures so that external plugins doesn't have to care about
bluez data structures too much and we can avoid exporting str2ba() and
other similar functions as global symbols in bluetooth.ver.

I hope you could comment on the patches and give me directions of how
you want me to do them if they are not OK as they are.

Once these —or equivalent— are merged then getting in the
playstation-peripheral plugin will take a very short time.

Thanks a lot,
   Antonio

[1] http://article.gmane.org/gmane.linux.bluez.kernel/16540

Changes since the RFC:
  - fixed a memory leak in btd_manager_get_default_adapter_str(), thanks to
    Daninele Forsi for looking at the code.
  - Removed unnecessary steps in btd_device_set_trusted()
  - Allow passing vendor_id_source in btd_device_set_trusted()

Antonio Ospite (3):
  manager: add a btd_manager_get_default_adapter_str() call
  storage: add a btd_store_record_string() call
  device: add a btd_device_set_trusted() call

 src/device.c  |   62 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/device.h  |    8 +++++++
 src/manager.c |   21 +++++++++++++++++++
 src/manager.h |    1 +
 src/storage.c |   17 +++++++++++++++
 src/storage.h |    1 +
 6 files changed, 110 insertions(+), 0 deletions(-)

-- 
Antonio Ospite
http://ao2.it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

[PATCH BlueZ 1/3] manager: add a btd_manager_get_default_adapter_str() call

[Antonio Ospite]

Add a new btd_* call to get the default adapter address as a string,
meant to be used by _external_ plugins.
---
 src/manager.c |   21 +++++++++++++++++++++
 src/manager.h |    1 +
 2 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/src/manager.c b/src/manager.c
index 1d44c66..64f7f2c 100644
--- a/src/manager.c
+++ b/src/manager.c
@@ -270,6 +270,27 @@ struct btd_adapter *manager_get_default_adapter(void)
 	return manager_find_adapter_by_id(default_adapter_id);
 }
 
+char *btd_manager_get_default_adapter_str(void)
+{
+	struct btd_adapter *adapter;
+	bdaddr_t adapter_bdaddr;
+	char *str;
+
+	adapter = manager_get_default_adapter();
+	if (adapter == NULL) {
+		return NULL;
+	}
+
+	adapter_get_address(adapter, &adapter_bdaddr);
+
+	str = bt_malloc(18);
+	if (str == NULL)
+		return NULL;
+
+	ba2str(&adapter_bdaddr, str);
+	return str;
+}
+
 static void manager_remove_adapter(struct btd_adapter *adapter)
 {
 	uint16_t dev_id = adapter_get_dev_id(adapter);
diff --git a/src/manager.h b/src/manager.h
index 4f92d2f..91d9894 100644
--- a/src/manager.h
+++ b/src/manager.h
@@ -36,6 +36,7 @@ const char *manager_get_base_path(void);
 struct btd_adapter *manager_find_adapter(const bdaddr_t *sba);
 struct btd_adapter *manager_find_adapter_by_id(int id);
 struct btd_adapter *manager_get_default_adapter(void);
+char *btd_manager_get_default_adapter_str(void);
 void manager_foreach_adapter(adapter_cb func, gpointer user_data);
 GSList *manager_get_adapters(void);
 struct btd_adapter *btd_manager_register_adapter(int id);
-- 
1.7.9

[PATCH BlueZ 2/3] storage: add a btd_store_record_string() call

[Antonio Ospite]

Add a new btd_* call to store an SDP record passed as a string, meant to
be used by _external_ plugins.
---
 src/storage.c |   17 +++++++++++++++++
 src/storage.h |    1 +
 2 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/src/storage.c b/src/storage.c
index a65cee4..e15705c 100644
--- a/src/storage.c
+++ b/src/storage.c
@@ -785,6 +785,23 @@ int store_record(const gchar *src, const gchar *dst, sdp_record_t *rec)
 	return err;
 }
 
+int btd_store_record_string(const gchar *src,
+			    const gchar *dst,
+			    const gchar *record_str)
+{
+	sdp_record_t *record;
+	int ret = 0;
+
+	record = record_from_string(record_str);
+	if (record == NULL) {
+		return -ENOMEM;
+	}
+	ret = store_record(src, dst, record);
+	sdp_record_free(record);
+
+	return ret;
+}
+
 sdp_record_t *record_from_string(const gchar *str)
 {
 	sdp_record_t *rec;
diff --git a/src/storage.h b/src/storage.h
index 2f145d0..eb61e47 100644
--- a/src/storage.h
+++ b/src/storage.h
@@ -56,6 +56,7 @@ int write_trust(const char *src, const char *addr, const char *service, gboolean
 int write_device_profiles(bdaddr_t *src, bdaddr_t *dst, const char *profiles);
 int delete_entry(bdaddr_t *src, const char *storage, const char *key);
 int store_record(const gchar *src, const gchar *dst, sdp_record_t *rec);
+int btd_store_record_string(const gchar *src, const gchar *dst, const gchar *record_str);
 sdp_record_t *record_from_string(const gchar *str);
 sdp_record_t *fetch_record(const gchar *src, const gchar *dst, const uint32_t handle);
 int delete_record(const gchar *src, const gchar *dst, const uint32_t handle);
-- 
1.7.9

[PATCH BlueZ 3/3] device: add a btd_device_set_trusted() call

[Antonio Ospite]

Add a new btd_* call to set a device as trusted, meant to be used by
_external_ plugins.
---
 src/device.c |   62 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/device.h |    8 +++++++
 2 files changed, 70 insertions(+), 0 deletions(-)

diff --git a/src/device.c b/src/device.c
index 3466adb..876e958 100644
--- a/src/device.c
+++ b/src/device.c
@@ -49,6 +49,7 @@
 #include "att.h"
 #include "hcid.h"
 #include "adapter.h"
+#include "manager.h"
 #include "gattrib.h"
 #include "attio.h"
 #include "device.h"
@@ -2767,6 +2768,67 @@ GSList *btd_device_get_primaries(struct btd_device *device)
 	return device->primaries;
 }
 
+int btd_device_set_trusted(const char *adapter_address,
+			   const char *device_address,
+			   char *name,
+			   uint16_t vendor_id_source,
+			   uint16_t vendor_id,
+			   uint16_t product_id,
+			   uint16_t version_id,
+			   const char *uuid)
+{
+	struct btd_adapter *adapter;
+	struct btd_device *device;
+	DBusConnection *conn;
+	bdaddr_t src;
+	bdaddr_t dst;
+	int ret = 0;
+
+	str2ba(adapter_address, &src);
+	str2ba(device_address, &dst);
+
+	/* Set the device id */
+	store_device_id(adapter_address, device_address, vendor_id_source, vendor_id,
+			product_id, version_id);
+	/* Don't write a profile here,
+	 * it will be updated when the device connects */
+
+	write_trust(adapter_address, device_address, "[all]", TRUE);
+
+	conn = dbus_bus_get(DBUS_BUS_SYSTEM, NULL);
+	if (conn == NULL) {
+		DBG("Failed to get on the bus");
+		ret = -EPERM;
+		goto out;
+	}
+
+	adapter = manager_find_adapter(&src);
+	if (adapter == NULL) {
+		DBG("Failed to get the adapter");
+		ret = -EPERM;
+		goto out_dbus_unref;
+	}
+
+	/* This is needed: adapter_find_device() wouldn't need a Dbus
+	 * connection but it would not be enough as it only searches for
+	 * already existing devices, while adapter_get_device() will create a
+	 * new device if necessary.
+	 */
+	device = adapter_get_device(conn, adapter, device_address);
+	if (device == NULL) {
+		DBG("Failed to get the device");
+		ret = -ENODEV;
+		goto out_dbus_unref;
+	}
+
+	btd_device_add_uuid(device, uuid);
+
+out_dbus_unref:
+	dbus_connection_unref(conn);
+out:
+	return ret;
+}
+
 void btd_device_add_uuid(struct btd_device *device, const char *uuid)
 {
 	GSList *uuid_list;
diff --git a/src/device.h b/src/device.h
index 7cb9bb2..443b95a 100644
--- a/src/device.h
+++ b/src/device.h
@@ -55,6 +55,14 @@ void device_register_services(DBusConnection *conn, struct btd_device *device,
 						GSList *prim_list, int psm);
 GSList *device_services_from_record(struct btd_device *device,
 							GSList *profiles);
+int btd_device_set_trusted(const char *adapter_address,
+			   const char *device_address,
+			   char *name,
+			   uint16_t vendor_id_source,
+			   uint16_t vendor_id,
+			   uint16_t product_id,
+			   uint16_t version_id,
+			   const char *uuid);
 void btd_device_add_uuid(struct btd_device *device, const char *uuid);
 struct btd_adapter *device_get_adapter(struct btd_device *device);
 void device_get_address(struct btd_device *device, bdaddr_t *bdaddr,
-- 
1.7.9

Re: [PATCH BlueZ 0/3] Prerequisites for playstation-peripheral _external_ plugin

[Johan Hedberg]

Hi Antonio,

On Mon, Feb 13, 2012, Antonio Ospite wrote:
> I proposed these changes back in September[1] but they didn't receive
> a lot of comments, I know they are not very interesting to most of you,
> but let me repeat the rationale behind them.
> 
>   1. It was decided to have the playstation-peripheral as an external
>      plugin because it is going to be used by few people and it is not
>      worth to keep it in the bluetoothd binary. Distributors could
>      choose to put external plugins in their own packages so to provide
>      a install-time choice instead of a compile-time choice to use the
>      plugin services.
> 
>   2. external plugins can only access symbols exported as global in
>      src/bluetooth.ver, like btd_* and very few others.

libbluetooth public symbols should be available for anyone, including
plugins. If they're not that needs fixing. As for the other ones missing
a btd_ prefix you should just fix those too, i.e. this new _str function
shouldn't be needed at all.

Btw, where's the code for this playstation-peripheral plugin? That would
be important to see in order to better understand the use cases these
new bluetoothd APIs are needed for.

Johan

Re: [PATCH BlueZ 3/3] device: add a btd_device_set_trusted() call

[Johan Hedberg]

Hi Antonio,

On Mon, Feb 13, 2012, Antonio Ospite wrote:
> +int btd_device_set_trusted(const char *adapter_address,
> +			   const char *device_address,
> +			   char *name,
> +			   uint16_t vendor_id_source,
> +			   uint16_t vendor_id,
> +			   uint16_t product_id,
> +			   uint16_t version_id,
> +			   const char *uuid)
> +{

If you have a function called set_trusted then that's *all* it should
do. You have six other parameters there that have *nothing* to do with
the function name. Also, as I mentioned in my previous email the full
use case for all of these should be understood (which also implies the
code that uses them being available) before anything gets added to the
bluetoothd exported API.

Johan

Re: [PATCH BlueZ 3/3] device: add a btd_device_set_trusted() call

[Antonio Ospite]

[-- Attachment #1: Type: text/plain, Size: 1903 bytes --]

On Mon, 16 Apr 2012 15:57:48 +0300
Johan Hedberg <johan.hedberg@gmail.com> wrote:

> Hi Antonio,
> 
> On Mon, Feb 13, 2012, Antonio Ospite wrote:
> > +int btd_device_set_trusted(const char *adapter_address,
> > +			   const char *device_address,
> > +			   char *name,
> > +			   uint16_t vendor_id_source,
> > +			   uint16_t vendor_id,
> > +			   uint16_t product_id,
> > +			   uint16_t version_id,
> > +			   const char *uuid)
> > +{
> 
> If you have a function called set_trusted then that's *all* it should
> do. You have six other parameters there that have *nothing* to do with
> the function name.

Well that exposes my confusion/ignorance about what a "trusted device"
is, from what see, to make a device look fully trusted to BlueZ, it is
not enough to just call:

	write_trust(adapter_address, device_address, "[all]", TRUE);

a bunch of other properties are needed (uuids, sdp records), maybe the
problem is about defining what I want, maybe I want a
_statically_trusted_ device? I don't know if that even makes sense in
bluetooth terminology.

The point here is that we are setting these parameters BEFORE the
devices connects via bluetooth, in fact they are set when the device is
connected via USB for the cable pairing step.

> Also, as I mentioned in my previous email the full
> use case for all of these should be understood (which also implies the
> code that uses them being available) before anything gets added to the
> bluetoothd exported API.

You are right, of course, I'll prepare a more detailed text which
explains WHAT I need to do and WHY, and hopefully you can guide me
about HOW to do that properly.

Thanks,
   Antonio

-- 
Antonio Ospite
http://ao2.it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]