Closed Bug 1129257 Opened 5 years ago Closed 5 years ago

[Bluetooth][Daemon] Correct the data type of |Channel| with |int32_t| in Bluetooth Socket HAL.

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:2.2+, firefox36 wontfix, firefox37 wontfix, firefox38 fixed, b2g-v2.2 fixed, b2g-master fixed)

RESOLVED FIXED
2.2 S5 (6feb)
blocking-b2g 2.2+
Tracking Status
firefox36 --- wontfix
firefox37 --- wontfix
firefox38 --- fixed
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: brsun, Assigned: brsun)

References

Details

Attachments

(2 files, 1 obsolete file)

This bug is originated from [1]. The BlueZ doc says |Channel| is 2 octets, but it is |int32_t| inside source codes of BlueZ. See also: [2][3].

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1124556#c15
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1124556#c17
[3] https://bugzilla.mozilla.org/show_bug.cgi?id=1124556#c18
Attachment #8558975 - Flags: review?(tzimmermann)
Comment on attachment 8558975 [details] [diff] [review]
bug1129257_bluetoothd_socket_channel_int32_t.patch

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

r+ with the comments addressed.

::: dom/bluetooth/bluedroid/BluetoothDaemonHelpers.cpp
@@ +72,5 @@
>  
>  nsresult
> +Convert(int aIn, int32_t& aOut)
> +{
> +  if (NS_WARN_IF(!mozilla::detail::IsInRange<int32_t>(aIn))) {

The rest of the file uses C++' 'std::limit'. If there's no coding style requirement, please use limit as well.

@@ +81,5 @@
> +  return NS_OK;
> +}
> +
> +nsresult
> +Convert(int aIn, uint32_t& aOut)

Please remove this function after changing BluetoothDaemonSocketInterface.

::: dom/bluetooth/bluedroid/BluetoothDaemonSocketInterface.cpp
@@ +35,5 @@
>    nsresult rv = PackPDU(
>      aType,
>      PackConversion<nsAString, BluetoothServiceName>(aServiceName),
>      PackArray<uint8_t>(aServiceUuid, 16),
> +    PackConversion<int, uint32_t>(aChannel),

This should be |int32_t| as well. Using 'unsigned' is a bug in the current code.
Attachment #8558975 - Flags: review?(tzimmermann) → review+
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #3)
> Comment on attachment 8558975 [details] [diff] [review]
> bug1129257_bluetoothd_socket_channel_int32_t.patch
> 
> Review of attachment 8558975 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with the comments addressed.
> 
> ::: dom/bluetooth/bluedroid/BluetoothDaemonHelpers.cpp
> @@ +72,5 @@
> >  
> >  nsresult
> > +Convert(int aIn, int32_t& aOut)
> > +{
> > +  if (NS_WARN_IF(!mozilla::detail::IsInRange<int32_t>(aIn))) {
> 
> The rest of the file uses C++' 'std::limit'. If there's no coding style
> requirement, please use limit as well.
> 

There will be one compile warning while comparing a signed integer value with a unsigned integer value by using original coding style, and this warning will result a compile error if warning-as-error is on.
(In reply to Bruce Sun [:brsun] from comment #5)
> There will be one compile warning while comparing a signed integer value
> with a unsigned integer value by using original coding style, and this
> warning will result a compile error if warning-as-error is on.

BTW, suppose we can avoid this issue if we don't convert between |int| and |uint32_t|. I'll give it a try later.
Address suggestions on comment 3.
Attachment #8558975 - Attachment is obsolete: true
Attachment #8559015 - Flags: review+
blocking-b2g: --- → 2.2?
Keywords: checkin-needed
(In reply to Bruce Sun [:brsun] from comment #8)
> Waiting for the result of TBPL:
>  - https://treeherder.mozilla.org/#/jobs?repo=try&revision=2525ee84e226
>  - https://tbpl.mozilla.org/?tree=Try&rev=2525ee84e226

TBPL results seem good.
Bruce,

are you aware that all your fixes need to go into bluetooth2/ as well?
Yes, I know that...I just don't have the courage to handle those stuffs yet...

By the way, I would suggest to handle bluetooth2 after bluetooth1 is stable and has been verified properly. We have no ways to make sure the porting tasks from bluetooth1 to bluetooth2 are properly handled in the current stage.
Blocks: 1129846
(In reply to Bruce Sun [:brsun] from comment #11)
> Yes, I know that...I just don't have the courage to handle those stuffs
> yet...
> 
> By the way, I would suggest to handle bluetooth2 after bluetooth1 is stable
> and has been verified properly. We have no ways to make sure the porting
> tasks from bluetooth1 to bluetooth2 are properly handled in the current
> stage.

Yeah, I know. :( The situation around bluetooth2 is dire and I expect regressions left and right, once it gets merged officially.

I wrote all of the current backend code for HAL and bluetoothd and have been porting it 'blindly' to bluetooth2 by renaming file paths in the patch files and compiling the results. I'm glad that the backend code doesn't conflict much with the rest of the bluetooth2 changes.

And there is no way to really test anything in bluetooth2.
Duplicate of this bug: 1124556
Triage: blocking
blocking-b2g: 2.2? → 2.2+
Comment on attachment 8559015 [details] [diff] [review]
bug1129257_bluetoothd_socket_channel_int32_t.commit.patch

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 1124556
User impact if declined: The behavior of device connection triggered by NFC is wrong.
Testing completed: Manually test locally.
Risk to taking this patch (and alternatives if risky): Low. But it's worth noting that |libxul.so| and |bluetoothd| are needed to be updated concurrently. File transfer functions will break if only one side (gecko or bluetoothd) has been updated without corresponding update on the other side (bluetoothd or gecko).
String or UUID changes made by this patch: N/A
Attachment #8559015 - Flags: approval-mozilla-b2g37?
Attachment #8558976 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Attachment #8559015 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
This was merged a few days ago and got missed.

https://hg.mozilla.org/mozilla-central/rev/38d3cf148195
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.