From: Mikel Astiz <[email protected]>
Vinicius reported this issue and tried to fix it in his patch "profile: Create a service for incomming connections".
As compared to his original patch, this patchset avoid creating an additional service instance (which should already be created) and instead performs a search within the list of existing services for the given device.
Patch 2/3 is the actual fix which makes use of patch 1/1.
Patch 3/3 is an attempt to make the code more robust, but I'm not 100% sure if there are cases where the device can be set to NULL as handled in the upstream create_conn() body as first introduced in 86842030039583d092a1d3ce5004e7bc5b7d2b01.
Mikel Astiz (3):
device: Add function to find a given service
profile: Fix remotely initiated connections without service
profile: Error-cases for incoming connections
src/device.c | 16 ++++++++++++++++
src/device.h | 3 +++
src/profile.c | 26 +++++++++++++++++++++-----
3 files changed, 40 insertions(+), 5 deletions(-)
--
1.8.1.4
Hi Johan, Vinicius,
On Tue, May 7, 2013 at 5:06 PM, Johan Hedberg <[email protected]> wrote:
> Hi Vinicius,
>
> On Tue, May 07, 2013, Vinicius Gomes wrote:
>> > As compared to his original patch, this patchset avoid creating an
>> > additional service instance (which should already be created) and
>> > instead performs a search within the list of existing services for
>> > the given device.
>>
>> In the AG case when the external profile registers itself with the
>> "server" role, the service doesn't get created.
>
> Hmm? HFP profile instances should be registered without any specific
> "Role" property in RegisterProfile since both client and server sides
> are required.
>
>> Nowadays, the service is created when the service is probed (with
>> requires profile->device_probe), which doesn't happen for "adapter"
>> services. I am still not sure about the right place/time to create the
>> service for these cases.
>
> To my understanding btd_service is only for describing remote services
> and not local ones, i.e. those that we can connect to as a client. I may
> have misunderstood something about this though (for which Mikel is free
> to correct me).
The statement above is correct, btd_service is designed to represent
remote services only.
The confusion here might be caused by the fact that profile.c makes
use of struct ext_io to represent both local and remote "services",
but I believe this is an unfortunate implementation detail.
Cheers,
Mikel
Hi Vinicius,
On Tue, May 07, 2013, Vinicius Gomes wrote:
> > As compared to his original patch, this patchset avoid creating an
> > additional service instance (which should already be created) and
> > instead performs a search within the list of existing services for
> > the given device.
>
> In the AG case when the external profile registers itself with the
> "server" role, the service doesn't get created.
Hmm? HFP profile instances should be registered without any specific
"Role" property in RegisterProfile since both client and server sides
are required.
> Nowadays, the service is created when the service is probed (with
> requires profile->device_probe), which doesn't happen for "adapter"
> services. I am still not sure about the right place/time to create the
> service for these cases.
To my understanding btd_service is only for describing remote services
and not local ones, i.e. those that we can connect to as a client. I may
have misunderstood something about this though (for which Mikel is free
to correct me).
Johan
Hi Mikel,
On Tue, May 7, 2013 at 4:44 AM, Mikel Astiz <[email protected]> wrote:
> From: Mikel Astiz <[email protected]>
>
> Vinicius reported this issue and tried to fix it in his patch "profile: Create a service for incomming connections".
>
> As compared to his original patch, this patchset avoid creating an additional service instance (which should already be created) and instead performs a search within the list of existing services for the given device.
In the AG case when the external profile registers itself with the
"server" role, the service doesn't get created.
Nowadays, the service is created when the service is probed (with
requires profile->device_probe), which doesn't happen for "adapter"
services. I am still not sure about the right place/time to create the
service for these cases.
But this is another problem. Apart from the minor issue pointed in
3/3, you have my Ack.
>
> Patch 2/3 is the actual fix which makes use of patch 1/1.
>
> Patch 3/3 is an attempt to make the code more robust, but I'm not 100% sure if there are cases where the device can be set to NULL as handled in the upstream create_conn() body as first introduced in 86842030039583d092a1d3ce5004e7bc5b7d2b01.
>
> Mikel Astiz (3):
> device: Add function to find a given service
> profile: Fix remotely initiated connections without service
> profile: Error-cases for incoming connections
>
> src/device.c | 16 ++++++++++++++++
> src/device.h | 3 +++
> src/profile.c | 26 +++++++++++++++++++++-----
> 3 files changed, 40 insertions(+), 5 deletions(-)
>
> --
> 1.8.1.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Cheers,
--
Vinicius
Hi Mikel,
On Tue, May 7, 2013 at 4:44 AM, Mikel Astiz <[email protected]> wrote:
> From: Mikel Astiz <[email protected]>
>
> When an incoming connection attempt is received, if the corresponding
> device or service is not found, handle the error-case.
> ---
> src/profile.c | 31 ++++++++++++++++++++-----------
> 1 file changed, 20 insertions(+), 11 deletions(-)
>
> diff --git a/src/profile.c b/src/profile.c
> index 6a71627..aab7e33 100644
> --- a/src/profile.c
> +++ b/src/profile.c
> @@ -1048,22 +1048,26 @@ static struct ext_io *create_conn(struct ext_io *server, GIOChannel *io,
> struct ext_io *conn;
> GIOCondition cond;
>
> + device = adapter_find_device(server->adapter, dst);
> + if (device == NULL) {
> + error("%s device %s not found", server->ext->name, src);
src is a bdaddr_t, so it would be better converted to a proper string.
> + return NULL;
> + }
> +
> + service = btd_device_get_service(device, server->ext->remote_uuid);
> + if (service == NULL) {
> + error("%s service not found for device %s", server->ext->name,
> + src);
Same here.
> + return NULL;
> + }
> +
> conn = g_new0(struct ext_io, 1);
> conn->io = g_io_channel_ref(io);
> conn->proto = server->proto;
> conn->ext = server->ext;
> conn->adapter = btd_adapter_ref(server->adapter);
> -
> - device = adapter_find_device(server->adapter, dst);
> -
> - if (device) {
> - conn->device = btd_device_ref(device);
> -
> - service = btd_device_get_service(device,
> - server->ext->remote_uuid);
> - if (service)
> - conn->service = btd_service_ref(service);
> - }
> + conn->device = btd_device_ref(device);
> + conn->service = btd_service_ref(service);
>
> cond = G_IO_HUP | G_IO_ERR | G_IO_NVAL;
> conn->io_id = g_io_add_watch(io, cond, ext_io_disconnected, conn);
> @@ -1137,6 +1141,8 @@ static void ext_confirm(GIOChannel *io, gpointer user_data)
> DBG("incoming connect from %s", addr);
>
> conn = create_conn(server, io, &src, &dst);
> + if (conn == NULL)
> + return;
>
> conn->auth_id = btd_request_authorization(&src, &dst, uuid, ext_auth,
> conn);
> @@ -1175,6 +1181,9 @@ static void ext_direct_connect(GIOChannel *io, GError *err, gpointer user_data)
> }
>
> conn = create_conn(server, io, &src, &dst);
> + if (conn == NULL)
> + return;
> +
> ext->conns = g_slist_append(ext->conns, conn);
>
> ext_connect(io, err, conn);
> --
> 1.8.1.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Cheers,
--
Vinicius
From: Mikel Astiz <[email protected]>
Remotely initiated connections need to be associated to a service
instance to avoid the following crash as reported by Vinicius Costa:
bluetoothd[14366]: src/profile.c:ext_confirm() incoming connect from A0:4E:04:F6:F5:05
bluetoothd[14366]: src/profile.c:ext_confirm() hfp_hf authorizing connection from A0:4E:04:F6:F5:05
bluetoothd[14366]: src/agent.c:agent_ref() 0x6a85e0: ref=2
bluetoothd[14366]: src/agent.c:agent_authorize_service() authorize service request was sent for /org/bluez/hci0/dev_A0_4E_04_F6_F5_05
bluetoothd[14366]: src/profile.c:ext_svc_complete() Services resolved for A0:4E:04:F6:F5:05
bluetoothd[14366]: src/profile.c:ext_svc_complete() Services resolved but still waiting for authorization
bluetoothd[14366]: src/profile.c:ext_auth() A0:4E:04:F6:F5:05 authorized to connect to hfp_hf
bluetoothd[14366]: src/agent.c:agent_unref() 0x6a85e0: ref=1
bluetoothd[14366]: src/profile.c:ext_connect() hfp_hf connected to A0:4E:04:F6:F5:05
Program received signal SIGSEGV, Segmentation fault.
btd_service_connecting_complete (service=0x0, err=err@entry=0) at src/service.c:315
315 if (service->state != BTD_SERVICE_STATE_DISCONNECTED &&
---
src/profile.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/src/profile.c b/src/profile.c
index 0500983..6a71627 100644
--- a/src/profile.c
+++ b/src/profile.c
@@ -1044,6 +1044,7 @@ static struct ext_io *create_conn(struct ext_io *server, GIOChannel *io,
bdaddr_t *src, bdaddr_t *dst)
{
struct btd_device *device;
+ struct btd_service *service;
struct ext_io *conn;
GIOCondition cond;
@@ -1055,9 +1056,15 @@ static struct ext_io *create_conn(struct ext_io *server, GIOChannel *io,
device = adapter_find_device(server->adapter, dst);
- if (device)
+ if (device) {
conn->device = btd_device_ref(device);
+ service = btd_device_get_service(device,
+ server->ext->remote_uuid);
+ if (service)
+ conn->service = btd_service_ref(service);
+ }
+
cond = G_IO_HUP | G_IO_ERR | G_IO_NVAL;
conn->io_id = g_io_add_watch(io, cond, ext_io_disconnected, conn);
--
1.8.1.4
From: Mikel Astiz <[email protected]>
When an incoming connection attempt is received, if the corresponding
device or service is not found, handle the error-case.
---
src/profile.c | 31 ++++++++++++++++++++-----------
1 file changed, 20 insertions(+), 11 deletions(-)
diff --git a/src/profile.c b/src/profile.c
index 6a71627..aab7e33 100644
--- a/src/profile.c
+++ b/src/profile.c
@@ -1048,22 +1048,26 @@ static struct ext_io *create_conn(struct ext_io *server, GIOChannel *io,
struct ext_io *conn;
GIOCondition cond;
+ device = adapter_find_device(server->adapter, dst);
+ if (device == NULL) {
+ error("%s device %s not found", server->ext->name, src);
+ return NULL;
+ }
+
+ service = btd_device_get_service(device, server->ext->remote_uuid);
+ if (service == NULL) {
+ error("%s service not found for device %s", server->ext->name,
+ src);
+ return NULL;
+ }
+
conn = g_new0(struct ext_io, 1);
conn->io = g_io_channel_ref(io);
conn->proto = server->proto;
conn->ext = server->ext;
conn->adapter = btd_adapter_ref(server->adapter);
-
- device = adapter_find_device(server->adapter, dst);
-
- if (device) {
- conn->device = btd_device_ref(device);
-
- service = btd_device_get_service(device,
- server->ext->remote_uuid);
- if (service)
- conn->service = btd_service_ref(service);
- }
+ conn->device = btd_device_ref(device);
+ conn->service = btd_service_ref(service);
cond = G_IO_HUP | G_IO_ERR | G_IO_NVAL;
conn->io_id = g_io_add_watch(io, cond, ext_io_disconnected, conn);
@@ -1137,6 +1141,8 @@ static void ext_confirm(GIOChannel *io, gpointer user_data)
DBG("incoming connect from %s", addr);
conn = create_conn(server, io, &src, &dst);
+ if (conn == NULL)
+ return;
conn->auth_id = btd_request_authorization(&src, &dst, uuid, ext_auth,
conn);
@@ -1175,6 +1181,9 @@ static void ext_direct_connect(GIOChannel *io, GError *err, gpointer user_data)
}
conn = create_conn(server, io, &src, &dst);
+ if (conn == NULL)
+ return;
+
ext->conns = g_slist_append(ext->conns, conn);
ext_connect(io, err, conn);
--
1.8.1.4
From: Mikel Astiz <[email protected]>
Search within the list of services for a given remote UUID.
---
src/device.c | 16 ++++++++++++++++
src/device.h | 3 +++
2 files changed, 19 insertions(+)
diff --git a/src/device.c b/src/device.c
index ff4c553..14c3e81 100644
--- a/src/device.c
+++ b/src/device.c
@@ -4376,6 +4376,22 @@ static void service_state_changed(struct btd_service *service,
device_profile_disconnected(device, profile, err);
}
+struct btd_service *btd_device_get_service(struct btd_device *dev,
+ const char *remote_uuid)
+{
+ GSList *l;
+
+ for (l = dev->services; l != NULL; l = g_slist_next(l)) {
+ struct btd_service *service = l->data;
+ struct btd_profile *p = btd_service_get_profile(service);
+
+ if (g_str_equal(p->remote_uuid, remote_uuid))
+ return service;
+ }
+
+ return NULL;
+}
+
void btd_device_init(void)
{
dbus_conn = btd_get_dbus_connection();
diff --git a/src/device.h b/src/device.h
index 7cd11af..70e1502 100644
--- a/src/device.h
+++ b/src/device.h
@@ -121,5 +121,8 @@ unsigned int device_wait_for_svc_complete(struct btd_device *dev,
bool device_remove_svc_complete_callback(struct btd_device *dev,
unsigned int id);
+struct btd_service *btd_device_get_service(struct btd_device *dev,
+ const char *remote_uuid);
+
void btd_device_init(void);
void btd_device_cleanup(void);
--
1.8.1.4