Closed Bug 1209469 Opened 9 years ago Closed 9 years ago

Expose Bluetooth backend types in interface

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

Product:
Firefox OS Graveyard
Component:
Bluetooth
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox44 fixed)

Status:
RESOLVED FIXED
Milestone:
FxOS-S9 (16Oct)
Tracking Flags:
Tracking Status
firefox44 --- fixed

People

(Reporter: tzimmermann, Assigned: tzimmermann)

References

Details

Attachments

(6 files, 7 obsolete files)

[01] Bug 1209469: Expose |BluetoothAclState| in Bluetooth backend interface
5.42 KB, patch
brsun
: review+
Details | Diff | Splinter Review
[02] Bug 1209469: Expose |BluetoothPinCode| in Bluetooth backend interface (v2)
7.43 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
[04] Bug 1209469: Expose |BluetoothServiceName| in Bluetooth backend interface (v2)
7.42 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
[05] Bug 1209469: Expose |BluetoothPropertyType| in Bluetooth backend interface (v1)
7.59 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
[03] Bug 1209469: Expose |BluetoothRemoteName| in Bluetooth backend interface (v3)
10.05 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
[06] Bug 1209469: Replace |BluetoothNamedValue| with |BluetoothProperty| in Bluetooth backend (v3)
14.83 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review 
This is a follow-up bug for bug 1207649. After the address strings have been removed from the Bluetooth backend interface, we can expose the remaining internal backend types in the interface and further simplify the conversion code.
Comment on attachment 8667233 [details] [diff] [review]
[01] Bug 1209469: Expose |BluetoothAclState| in Bluetooth backend interface

Review of attachment 8667233 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.
Attachment #8667233 - Flags: review?(brsun) → review+
Comment on attachment 8667234 [details] [diff] [review]
[02] Bug 1209469: Expose |BluetoothPinCode| in Bluetooth backend interface

Review of attachment 8667234 [details] [diff] [review]:
-----------------------------------------------------------------

Basically this patch looks good to me.

It seems most of the logic of |StringToPinCode(...)| comes from the |Conver(const nsAString& aIn, BluetoothPinCode& aOut)| function. Maybe this is a good timing to refine the conversion.

r=me if the following refinement of string-length checking/assignment could be addressed.

::: dom/bluetooth/common/BluetoothUtils.cpp
@@ +64,5 @@
> +nsresult
> +StringToPinCode(const nsAString& aString, BluetoothPinCode& aPinCode)
> +{
> +  if (aString.Length() > MOZ_ARRAY_LENGTH(aPinCode.mPinCode)) {
> +    BT_LOGR("Pin code string too long");

This compares the buffer length with the length of PinCode in UTF-16.

The length of string in UTF-16 might be different then the length of the string with the same characters in UTF-8. It could become shorter in UTF-8 after the conversion[1].

Base on Bluetooth Core Specification, Volume 3, Part C, Section 3.2.3.3, it would be more reasonable to compare the buffer length to the length of the converted string in UTF-8.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Guide/Internal_strings#Unicode_Conversion_ns*CString_vs._ns*String

@@ +71,5 @@
> +
> +  NS_ConvertUTF16toUTF8 stringUTF8(aString);
> +  const char* str = stringUTF8.get();
> +
> +  aPinCode.mLength = aString.Length();

It would be more reasonable to assign |aPinCode.mLength| with the length of |stringUTF8|.
Attachment #8667234 - Flags: review?(brsun) → review+
Comment on attachment 8667235 [details] [diff] [review]
[03] Bug 1209469: Expose |BluetoothRemoteName| in Bluetooth backend interface

Review of attachment 8667235 [details] [diff] [review]:
-----------------------------------------------------------------

Basically this patch looks to me.

It seems the logic of |RemoteNameToString(...)| comes from the |Conver(const BluetoothRemoteName& aIn, nsAString& aOut)| function. Maybe this is a good timing to refine the conversion.

Probably it would be good as well to set the last one byte of |BluetoothRemoteName.mName| (i.e. |BluetoothRemoteName.mName[248]|) to zero while unpacking[1]. But this modification is not needed to be contained in the patch if setting 249-th byte to zero is not so relevant to this bug from your perspective.

r=me if the following string terminator handling could be addressed.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/BluetoothDaemonHelpers.h?offset=0#494

::: dom/bluetooth/common/BluetoothUtils.cpp
@@ +94,5 @@
> +
> +  // We construct an nsCString here because the string
> +  // returned from the PDU is not 0-terminated.
> +  aString = NS_ConvertUTF8toUTF16(
> +    nsCString(name, strnlen(name, sizeof(aRemoteName.mName))));

Base on Bluetooth Core Specification 4.2, Volumn 3, Part C, Section 3.2.2.3, the maximum length of Bluetooth device name is up to 248 bytes. However, |sizeof(BluetoothRemoteName.mName)| is 249. It would be more reasonable to copy not more than 248 bytes to |nsCString|.

BTW, how about using |nsDependentCString| to reduce one copy overhead?
Attachment #8667235 - Flags: review?(brsun) → review+
Comment on attachment 8667236 [details] [diff] [review]
[04] Bug 1209469: Expose |BluetoothServiceName| in Bluetooth backend interface

Review of attachment 8667236 [details] [diff] [review]:
-----------------------------------------------------------------

Basically it looks good to me.

r=me if the following string terminator handling could be clarified.

::: dom/bluetooth/common/BluetoothUtils.cpp
@@ +105,5 @@
> +  NS_ConvertUTF16toUTF8 serviceNameUTF8(aString);
> +  const char* str = serviceNameUTF8.get();
> +  size_t len = strlen(str);
> +
> +  if (len > sizeof(aServiceName.mName)) {

From the source codes of BlueZ, |hal_cmd_socket_listen.name|[1] is designed to store the service name, and |rfcomm_listen()|[2] and |l2cap_listen()|[2] is designed to use this char buffer. In those codes, I didn't found any logic to handle the string without a 0-terminator. Probably it would be good to set the last byte of |BluetoothServiceName.mName| (i.e. |BluetoothServiceName.mName[255]|) to zero from the beginning. If this is reasonable to you, then this conditional would need to be changed from |>| to |>=|, and the following |memcpy()| would need to be refined a little bit as well.

Since you are more familiar with BlueZ, would you mind helping to double check how the string without a 0-terminator would be handled in BlueZ?

[1] android/hal-msg.h
[2] android/socket.c
Attachment #8667236 - Flags: review?(brsun) → review+
Comment on attachment 8667237 [details] [diff] [review]
[05] Bug 1209469: Expose |BluetoothPropertyType| in Bluetooth backend interface

Review of attachment 8667237 [details] [diff] [review]:
-----------------------------------------------------------------

Basically it looks good to me.

r=me if the following question would be clarified.

Could |Convert(const nsAString& aIn, BluetoothPropertyType& aOut)| be removed after |StringToPropertyType(...)| has been defined?

::: dom/bluetooth/common/BluetoothUtils.cpp
@@ +97,5 @@
> +  } else if (aString.EqualsLiteral("DiscoverableTimeout")) {
> +    aType = PROPERTY_ADAPTER_DISCOVERY_TIMEOUT;
> +  } else {
> +    BT_LOGR("Invalid property name: %s", NS_ConvertUTF16toUTF8(aString).get());
> +    aType = static_cast<BluetoothPropertyType>(0); // silences compiler warning

How about assigning |PROPERTY_UNKNOWN| instead of a plain value 0?
Attachment #8667237 - Flags: review?(brsun) → review+
Comment on attachment 8667238 [details] [diff] [review]
[06] Bug 1209469: Replace |BluetoothNamedValue| with |BluetoothProperty| in Bluetooth backend

Review of attachment 8667238 [details] [diff] [review]:
-----------------------------------------------------------------

Basically it looks good to me.

r=me if the following suggestion would be addressed and if the following questions would be clarified.

::: dom/bluetooth/bluedroid/BluetoothDaemonHelpers.cpp
@@ +1229,5 @@
> +  switch (aIn.mType) {
> +    case PROPERTY_BDNAME:
> +      /* fall through */
> +    case PROPERTY_REMOTE_FRIENDLY_NAME: {
> +        NS_ConvertUTF16toUTF8 stringUTF8(aIn.mString);

Although |NS_ConvertUTF16toUTF8| is a concrete class that can be declared as a local variable, it might not so intuitive to use it directly based on its naming. Suggest to use |nsCString| to store the converted string for clarity.

@@ +1246,5 @@
> +                   aIn.mUint32,
> +                   aPDU);
> +      break;
> +    case PROPERTY_ADAPTER_SCAN_MODE:
> +      rv = PackPDU(PackConversion<size_t, uint16_t>(4), // 4-byte value

How about not using a plain value 4 as the magic number? There are functions that can convert between |BluetoothScanMode| and |int32_t|, probably using |sizeof(int32_t)| would be more meanful. What do you think?

@@ +1251,5 @@
> +                   aIn.mScanMode,
> +                   aPDU);
> +      break;
> +    default:
> +      MOZ_CRASH("Invalid property for packing");

Most of the |PackPDU(...)| simply returns error if some intermediate conversions fail. Not sure if it is suitable to use such strong mechanism like |MOZ_CRASH(...)| here. How about simply returning an error like other |PackPDU(...)|?

::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
@@ +1047,5 @@
>    ENSURE_BLUETOOTH_IS_READY(aRunnable, NS_OK);
>  
> +  BluetoothProperty property;
> +  nsresult rv = NamedValueToProperty(aValue, property);
> +  if (NS_FAILED(rv)) {

Add |DispatchReplyError(aRunnable, ...)|.

::: dom/bluetooth/common/BluetoothUtils.cpp
@@ +113,5 @@
> +  if (NS_FAILED(rv)) {
> +    return rv;
> +  }
> +
> +  if (aValue.value().type() == BluetoothValue::Tuint32_t) {

Suggest to check |aProperty.mType| with desired types to make it more clear.

@@ +117,5 @@
> +  if (aValue.value().type() == BluetoothValue::Tuint32_t) {
> +    // Set discoverable timeout
> +    aProperty.mUint32 = aValue.value().get_uint32_t();
> +
> +  } else if (aValue.value().type() == BluetoothValue::TnsString) {

Suggest to check |aProperty.mType| with desired types to make it more clear.

@@ +121,5 @@
> +  } else if (aValue.value().type() == BluetoothValue::TnsString) {
> +    // Set name
> +    aProperty.mString = aValue.value().get_nsString();
> +
> +  } else if (aValue.value().type() == BluetoothValue::Tbool) {

Suggest to check |aProperty.mType| with desired types to make it more clear.

@@ +128,5 @@
> +      aProperty.mScanMode = SCAN_MODE_CONNECTABLE_DISCOVERABLE;
> +    } else {
> +      aProperty.mScanMode = SCAN_MODE_CONNECTABLE;
> +    }
> +  } else {

How about setting |aProperty.mType| back to |PROPERTY_UNKNOWN|?
Attachment #8667238 - Flags: review?(brsun) → review+
Hi Bruce,

thanks for your comments and digging through the specs. I fully agree with most of what you said. I created the new functions from the existing conversion functions. If they can be improved, we should do that. The whole \0-termination issue for these values has kept me thinking and it's good to have some clarification.

1) I'll fix the Pin length. For the record: the spec says that the pin is 16 byte long, so there's *no* \0 terminator.

2) The structure for the remote names (with duplicates Android) is 249 to hold the additional \0 byte. I guess we should put only 248 bytes into our structure and let conversion/pack/unpack functions handle the \0 bytes.

3) BlueZ uses the service name and requires an \0 at the end. I tracked the usage of 'name' into |sdp_data_alloc|, where BlueZ calls strlen to get the name's length. Again, I think we should put 255 bytes into our structure and let the conversion/pack/unpack functions handle the additional terminator.

>> +
>> +  // We construct an nsCString here because the string
>> +  // returned from the PDU is not 0-terminated.
>> +  aString = NS_ConvertUTF8toUTF16(
>> +    nsCString(name, strnlen(name, sizeof(aRemoteName.mName))));
>
> BTW, how about using |nsDependentCString| to reduce one copy overhead?

|nsDependentString| requires the \0 at the end of the string. I don't remember if the comment refers to an actual bug, or if it's just a safety measure. But from reading through [1], I think we can give the C data and the length directly to |NS_ConvertUTF8toUTF16|, and save the intermediate string object.

In any case, I wouldn't trust the BT drivers to give us a correctly terminated string in all cases. Since the remote name is external input, when in doubt, I'd rather keep the overhead if we're unsure than exposing Gecko to a potential string overflow.

[1] https://dxr.mozilla.org/mozilla-central/source/xpcom/string/nsString.h?offset=0#162
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #13)
> Hi Bruce,
> 
> thanks for your comments and digging through the specs. I fully agree with
> most of what you said. I created the new functions from the existing
> conversion functions. If they can be improved, we should do that. The whole
> \0-termination issue for these values has kept me thinking and it's good to
> have some clarification.
> 
> 1) I'll fix the Pin length. For the record: the spec says that the pin is 16
> byte long, so there's *no* \0 terminator.
> 
> 2) The structure for the remote names (with duplicates Android) is 249 to
> hold the additional \0 byte. I guess we should put only 248 bytes into our
> structure and let conversion/pack/unpack functions handle the \0 bytes.
> 
> 3) BlueZ uses the service name and requires an \0 at the end. I tracked the
> usage of 'name' into |sdp_data_alloc|, where BlueZ calls strlen to get the
> name's length. Again, I think we should put 255 bytes into our structure and
> let the conversion/pack/unpack functions handle the additional terminator.
> 

Keep one extra byte in our own structures might be easier to use trivial memory copy to manage the conversion. It does make sense as well that we only keep necessary bytes in our own structures, and then handle the additional terminator inside the conversion/pack/unpack functions. Both ways are OK form my perspective, as long as we won't have extra overhead to reallocate memory while dealing with the conversion.

> >> +
> >> +  // We construct an nsCString here because the string
> >> +  // returned from the PDU is not 0-terminated.
> >> +  aString = NS_ConvertUTF8toUTF16(
> >> +    nsCString(name, strnlen(name, sizeof(aRemoteName.mName))));
> >
> > BTW, how about using |nsDependentCString| to reduce one copy overhead?
> 
> |nsDependentString| requires the \0 at the end of the string. I don't
> remember if the comment refers to an actual bug, or if it's just a safety
> measure. But from reading through [1], I think we can give the C data and
> the length directly to |NS_ConvertUTF8toUTF16|, and save the intermediate
> string object.
> 
> In any case, I wouldn't trust the BT drivers to give us a correctly
> terminated string in all cases. Since the remote name is external input,
> when in doubt, I'd rather keep the overhead if we're unsure than exposing
> Gecko to a potential string overflow.
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/xpcom/string/nsString.
> h?offset=0#162

Agree. Using |nsCString| is more suitable in this case. Thanks for the clarification.
(In reply to Bruce Sun [:brsun] from comment #11)
> Comment on attachment 8667237 [details] [diff] [review]
> [05] Bug 1209469: Expose |BluetoothPropertyType| in Bluetooth backend
> 
> Could |Convert(const nsAString& aIn, BluetoothPropertyType& aOut)| be
> removed after |StringToPropertyType(...)| has been defined?

It's still required until patch [06] lands.
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #15)
> (In reply to Bruce Sun [:brsun] from comment #11)
> > Comment on attachment 8667237 [details] [diff] [review]
> > [05] Bug 1209469: Expose |BluetoothPropertyType| in Bluetooth backend
> > 
> > Could |Convert(const nsAString& aIn, BluetoothPropertyType& aOut)| be
> > removed after |StringToPropertyType(...)| has been defined?
> 
> It's still required until patch [06] lands.

Hmm, or maybe not. I'll have to check, but I'll removed it in case.
(In reply to Bruce Sun [:brsun] from comment #12)
> Comment on attachment 8667238 [details] [diff] [review]
> [06] Bug 1209469: Replace |BluetoothNamedValue| with |BluetoothProperty| in
> Bluetooth backend
> 
> > +    case PROPERTY_REMOTE_FRIENDLY_NAME: {
> > +        NS_ConvertUTF16toUTF8 stringUTF8(aIn.mString);
> 
> Although |NS_ConvertUTF16toUTF8| is a concrete class that can be declared as
> a local variable, it might not so intuitive to use it directly based on its
> naming. Suggest to use |nsCString| to store the converted string for clarity.

I'd like to keep this line, because it's minimal and clear. The affected code is small anyway.
Changes since v1:

  - rewrite |StringToPinCode| with cleaner code
  - fixed string-length bug
Attachment #8667234 - Attachment is obsolete: true
Attachment #8668407 - Flags: review+
Changes since v1:

  - remote names have a maximum length of 248 bytes
  - maintain \0 character while unpacking PDU, but not in structure
  - give remote-name buffer directly to |NS_ConvertUTF8toUTF16|
Attachment #8667235 - Attachment is obsolete: true
Attachment #8668410 - Flags: review+
Changes since v1:

  - service names contain 255 bytes
  - the \0 character is maintained when writing the PDU, but not in the structure
  - rewrite |StringToServiceName| with cleaner code
Attachment #8667236 - Attachment is obsolete: true
Attachment #8668412 - Flags: review+
Changes since v1:

  - assign |PROPERTY_UNKNOWN| if conversion fails
Attachment #8667237 - Attachment is obsolete: true
Attachment #8668414 - Flags: review+
Changes since v1:

  - get scan-mode size from int32_t
  - don't crash in release builds if properties are incorrect
  - dispatch error if packing fails
  - rewrite conversion to focus on property type (instead of value type)
  - abort conversion if property type and value type are not compatible
  - removed old property-type/string conversion
Attachment #8667238 - Attachment is obsolete: true
Attachment #8668418 - Flags: review+
Comment on attachment 8668410 [details] [diff] [review]
[03] Bug 1209469: Expose |BluetoothRemoteName| in Bluetooth backend interface (v2)

Review of attachment 8668410 [details] [diff] [review]:
-----------------------------------------------------------------

There might be one typo in comments.

::: dom/bluetooth/bluedroid/BluetoothDaemonHelpers.h
@@ +462,5 @@
> +  if (NS_FAILED(rv)) {
> +    return rv;
> +  }
> +  /* The PDU stores one extra byte for the leading \0 character. We
> +   * conumer the byte, but don't store the character.

nit: "conumer" or "consume"?
Comment on attachment 8668418 [details] [diff] [review]
[06] Bug 1209469: Replace |BluetoothNamedValue| with |BluetoothProperty| in Bluetooth backend (v2)

Review of attachment 8668418 [details] [diff] [review]:
-----------------------------------------------------------------

The if-condition would need to be corrected.

::: dom/bluetooth/common/BluetoothUtils.cpp
@@ +118,5 @@
> +      aProperty.mString = aValue.value().get_nsString();
> +      break;
> +
> +    case PROPERTY_ADAPTER_SCAN_MODE:
> +      if (aValue.value().type() != BluetoothValue::TnsString) {

nit: |nsString| or |bool|?

@@ +131,5 @@
> +      }
> +      break;
> +
> +    case PROPERTY_ADAPTER_DISCOVERY_TIMEOUT:
> +      if (aValue.value().type() != BluetoothValue::TnsString) {

nit: |nsString| or |uint32_t|?
Thanks! Will be fixed in next iteration.
Changes since v2:

  - fixed typo in comment
Attachment #8668410 - Attachment is obsolete: true
Attachment #8668853 - Flags: review+
Changes since v2:

  - fix verification of types in |BluetoothNamedValue|
Attachment #8668418 - Attachment is obsolete: true
Attachment #8668854 - Flags: review+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: