| Message ID | 20200501144037.1684-1-alainm@chromium.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [BlueZ,v1] shared/gatt-client:Ignore orphaned characteristics | expand |
Hi Alain, > The gatt discovery proceedure simplification to discover all > characteristics at once has exposed a device side issue where some > device implementation reports orphaned characteristics. While this > technically shouldn't be allowed, some instances of this were observed > (namely on some Android phones). > > The fix is to simply skip over orphaned characteristics and continue > with everything else that is valid. > > This has been tested as part of the Android CTS tests against an > affected platform and was confirmed to have worked around the issue. > --- > > src/shared/gatt-client.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c > index 963ad619f..d604c77a3 100644 > --- a/src/shared/gatt-client.c > +++ b/src/shared/gatt-client.c > @@ -632,7 +632,10 @@ static bool discover_descs(struct discovery_op *op, bool *discovering) > util_debug(client->debug_callback, client->debug_data, > "Failed to insert characteristic at 0x%04x", > chrc_data->value_handle); > - goto failed; > + > + /* Skip over invalid characteristic */ > + free(chrc_data); > + continue; > } should we add a warning here? And I think it would be good to have a bit more verbose comment why this is done this way instead of aborting. Regards Marcel
Hi Marcel, On Fri, May 1, 2020 at 12:26 PM Marcel Holtmann <marcel@holtmann.org> wrote: > > Hi Alain, > > > The gatt discovery proceedure simplification to discover all > > characteristics at once has exposed a device side issue where some > > device implementation reports orphaned characteristics. While this > > technically shouldn't be allowed, some instances of this were observed > > (namely on some Android phones). > > > > The fix is to simply skip over orphaned characteristics and continue > > with everything else that is valid. > > > > This has been tested as part of the Android CTS tests against an > > affected platform and was confirmed to have worked around the issue. > > --- > > > > src/shared/gatt-client.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c > > index 963ad619f..d604c77a3 100644 > > --- a/src/shared/gatt-client.c > > +++ b/src/shared/gatt-client.c > > @@ -632,7 +632,10 @@ static bool discover_descs(struct discovery_op *op, bool *discovering) > > util_debug(client->debug_callback, client->debug_data, > > "Failed to insert characteristic at 0x%04x", > > chrc_data->value_handle); > > - goto failed; > > + > > + /* Skip over invalid characteristic */ > > + free(chrc_data); > > + continue; > > } > > should we add a warning here? Is there a good way other than the util_debug mechanism to write a warning? > > And I think it would be good to have a bit more verbose comment why this is done this way instead of aborting. Ack, will be added in V2. > > Regards > > Marcel >
Hi Alain, >>> The gatt discovery proceedure simplification to discover all >>> characteristics at once has exposed a device side issue where some >>> device implementation reports orphaned characteristics. While this >>> technically shouldn't be allowed, some instances of this were observed >>> (namely on some Android phones). >>> >>> The fix is to simply skip over orphaned characteristics and continue >>> with everything else that is valid. >>> >>> This has been tested as part of the Android CTS tests against an >>> affected platform and was confirmed to have worked around the issue. >>> --- >>> >>> src/shared/gatt-client.c | 5 ++++- >>> 1 file changed, 4 insertions(+), 1 deletion(-) >>> >>> diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c >>> index 963ad619f..d604c77a3 100644 >>> --- a/src/shared/gatt-client.c >>> +++ b/src/shared/gatt-client.c >>> @@ -632,7 +632,10 @@ static bool discover_descs(struct discovery_op *op, bool *discovering) >>> util_debug(client->debug_callback, client->debug_data, >>> "Failed to insert characteristic at 0x%04x", >>> chrc_data->value_handle); >>> - goto failed; >>> + >>> + /* Skip over invalid characteristic */ >>> + free(chrc_data); >>> + continue; >>> } >> >> should we add a warning here? > Is there a good way other than the util_debug mechanism to write a warning? you could just use btd_warn() here. If this gets too noisy, we either remove it later or we introduce a btd_warn_ratelimited() or btd_warn_once(). Regards Marcel
On Fri, May 1, 2020 at 1:32 PM Marcel Holtmann <marcel@holtmann.org> wrote: > > Hi Alain, > > >>> The gatt discovery proceedure simplification to discover all > >>> characteristics at once has exposed a device side issue where some > >>> device implementation reports orphaned characteristics. While this > >>> technically shouldn't be allowed, some instances of this were observed > >>> (namely on some Android phones). > >>> > >>> The fix is to simply skip over orphaned characteristics and continue > >>> with everything else that is valid. > >>> > >>> This has been tested as part of the Android CTS tests against an > >>> affected platform and was confirmed to have worked around the issue. > >>> --- > >>> > >>> src/shared/gatt-client.c | 5 ++++- > >>> 1 file changed, 4 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c > >>> index 963ad619f..d604c77a3 100644 > >>> --- a/src/shared/gatt-client.c > >>> +++ b/src/shared/gatt-client.c > >>> @@ -632,7 +632,10 @@ static bool discover_descs(struct discovery_op *op, bool *discovering) > >>> util_debug(client->debug_callback, client->debug_data, > >>> "Failed to insert characteristic at 0x%04x", > >>> chrc_data->value_handle); > >>> - goto failed; > >>> + > >>> + /* Skip over invalid characteristic */ > >>> + free(chrc_data); > >>> + continue; > >>> } > >> > >> should we add a warning here? > > Is there a good way other than the util_debug mechanism to write a warning? > > you could just use btd_warn() here. If this gets too noisy, we either remove it later or we introduce a btd_warn_ratelimited() or btd_warn_once(). This being under src/shared, I wasn't sure if that was acceptable to add a btd dependency here, especially with the work to avoid it via the util_debug. Happy to do either way, just want to make sure the guidance is clear. > > Regards > > Marcel >
Hi Alain, >>>>> The gatt discovery proceedure simplification to discover all >>>>> characteristics at once has exposed a device side issue where some >>>>> device implementation reports orphaned characteristics. While this >>>>> technically shouldn't be allowed, some instances of this were observed >>>>> (namely on some Android phones). >>>>> >>>>> The fix is to simply skip over orphaned characteristics and continue >>>>> with everything else that is valid. >>>>> >>>>> This has been tested as part of the Android CTS tests against an >>>>> affected platform and was confirmed to have worked around the issue. >>>>> --- >>>>> >>>>> src/shared/gatt-client.c | 5 ++++- >>>>> 1 file changed, 4 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c >>>>> index 963ad619f..d604c77a3 100644 >>>>> --- a/src/shared/gatt-client.c >>>>> +++ b/src/shared/gatt-client.c >>>>> @@ -632,7 +632,10 @@ static bool discover_descs(struct discovery_op *op, bool *discovering) >>>>> util_debug(client->debug_callback, client->debug_data, >>>>> "Failed to insert characteristic at 0x%04x", >>>>> chrc_data->value_handle); >>>>> - goto failed; >>>>> + >>>>> + /* Skip over invalid characteristic */ >>>>> + free(chrc_data); >>>>> + continue; >>>>> } >>>> >>>> should we add a warning here? >>> Is there a good way other than the util_debug mechanism to write a warning? >> >> you could just use btd_warn() here. If this gets too noisy, we either remove it later or we introduce a btd_warn_ratelimited() or btd_warn_once(). > This being under src/shared, I wasn't sure if that was acceptable to > add a btd dependency here, especially with the work to avoid it via > the util_debug. Happy to do either way, just want to make sure the > guidance is clear. good point. My bad. You better leave the logging out then. Regards Marcel
Hi Marcel, Alain, On Fri, May 1, 2020 at 11:04 AM Marcel Holtmann <marcel@holtmann.org> wrote: > > Hi Alain, > > >>>>> The gatt discovery proceedure simplification to discover all > >>>>> characteristics at once has exposed a device side issue where some > >>>>> device implementation reports orphaned characteristics. While this > >>>>> technically shouldn't be allowed, some instances of this were observed > >>>>> (namely on some Android phones). > >>>>> > >>>>> The fix is to simply skip over orphaned characteristics and continue > >>>>> with everything else that is valid. > >>>>> > >>>>> This has been tested as part of the Android CTS tests against an > >>>>> affected platform and was confirmed to have worked around the issue. > >>>>> --- > >>>>> > >>>>> src/shared/gatt-client.c | 5 ++++- > >>>>> 1 file changed, 4 insertions(+), 1 deletion(-) > >>>>> > >>>>> diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c > >>>>> index 963ad619f..d604c77a3 100644 > >>>>> --- a/src/shared/gatt-client.c > >>>>> +++ b/src/shared/gatt-client.c > >>>>> @@ -632,7 +632,10 @@ static bool discover_descs(struct discovery_op *op, bool *discovering) > >>>>> util_debug(client->debug_callback, client->debug_data, > >>>>> "Failed to insert characteristic at 0x%04x", > >>>>> chrc_data->value_handle); > >>>>> - goto failed; > >>>>> + > >>>>> + /* Skip over invalid characteristic */ > >>>>> + free(chrc_data); > >>>>> + continue; > >>>>> } > >>>> > >>>> should we add a warning here? > >>> Is there a good way other than the util_debug mechanism to write a warning? > >> > >> you could just use btd_warn() here. If this gets too noisy, we either remove it later or we introduce a btd_warn_ratelimited() or btd_warn_once(). > > This being under src/shared, I wasn't sure if that was acceptable to > > add a btd dependency here, especially with the work to avoid it via > > the util_debug. Happy to do either way, just want to make sure the > > guidance is clear. > > good point. My bad. You better leave the logging out then. There is already some logging a little bit before so I guess we are covered here.
diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c index 963ad619f..d604c77a3 100644 --- a/src/shared/gatt-client.c +++ b/src/shared/gatt-client.c @@ -632,7 +632,10 @@ static bool discover_descs(struct discovery_op *op, bool *discovering) util_debug(client->debug_callback, client->debug_data, "Failed to insert characteristic at 0x%04x", chrc_data->value_handle); - goto failed; + + /* Skip over invalid characteristic */ + free(chrc_data); + continue; } if (gatt_db_attribute_get_handle(attr) !=