bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

[Bluetooth] Implement general-purpose runnables for Bluetooth backends

RESOLVED FIXED in 2.1 S8 (7Nov)

Status

Firefox OS
Bluetooth
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: tzimmermann, Assigned: tzimmermann)

Tracking

unspecified
2.1 S8 (7Nov)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 2 obsolete attachments)

The notification and interface runnables for the Bluetooth backends need a rework. The current implementation is backend-specific and not always easily discoverable.
Created attachment 8514238 [details] [diff] [review]
[01] Bug 1091577: Added |BluetoothDaemonPDU::GetHeader|
Attachment #8514238 - Flags: review?(shuang)
Attachment #8514238 - Flags: feedback?(btian)
Created attachment 8514239 [details] [diff] [review]
[02] Bug 1091577: Added general-purpose notification runnables for Bluetooth
Attachment #8514239 - Flags: review?(shuang)
Attachment #8514239 - Flags: feedback?(btian)
Created attachment 8514240 [details] [diff] [review]
[03] Bug 1091577: Use general-purpose notification runnable in Bluetooth daemon backend
Attachment #8514240 - Flags: review?(shuang)
Attachment #8514240 - Flags: feedback?(btian)
Created attachment 8514241 [details] [diff] [review]
[04] Bug 1091577: Use general-purpose result runnable in Bluetooth daemon backend
Attachment #8514241 - Flags: review?(shuang)
Attachment #8514241 - Flags: feedback?(btian)
Created attachment 8514242 [details] [diff] [review]
[05] Bug 1091577: Remove unused helpers from daemon backend
Attachment #8514242 - Flags: review?(shuang)
Attachment #8514242 - Flags: feedback?(btian)
Shawn,

This patch set fixes the mess around unpack functions that you noted in bug 1073548, makes a lot of code backend-independent, and improves error checks when parsing the HAL IPC protocol.
Blocks: 1091588
Blocks: 1088574
Comment on attachment 8514238 [details] [diff] [review]
[01] Bug 1091577: Added |BluetoothDaemonPDU::GetHeader|

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

::: ipc/bluetooth/BluetoothDaemonConnection.cpp
@@ +63,5 @@
> +                              uint16_t& aPayloadSize)
> +{
> +  memcpy(&aService, GetData(OFF_SERVICE), sizeof(aService));
> +  memcpy(&aOpcode, GetData(OFF_OPCODE), sizeof(aService));
> +  memcpy(&aPayloadSize, GetData(OFF_LENGTH), sizeof(aService));

it should be |sizeof(aPayloadSize)|, right?
Attachment #8514238 - Flags: review?(shuang)
(In reply to Shawn Huang [:shawnjohnjr] from comment #7)
> Comment on attachment 8514238 [details] [diff] [review]
> [01] Bug 1091577: Added |BluetoothDaemonPDU::GetHeader|
> 
> Review of attachment 8514238 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: ipc/bluetooth/BluetoothDaemonConnection.cpp
> @@ +63,5 @@
> > +                              uint16_t& aPayloadSize)
> > +{
> > +  memcpy(&aService, GetData(OFF_SERVICE), sizeof(aService));
> > +  memcpy(&aOpcode, GetData(OFF_OPCODE), sizeof(aService));
> > +  memcpy(&aPayloadSize, GetData(OFF_LENGTH), sizeof(aService));
> 
> it should be |sizeof(aPayloadSize)|, right?

Copy-pasta mistake. m(

Yes! Thank you.
Comment on attachment 8514239 [details] [diff] [review]
[02] Bug 1091577: Added general-purpose notification runnables for Bluetooth

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

It looks good to me.
Attachment #8514239 - Flags: review?(shuang) → review+
Attachment #8514242 - Flags: review?(shuang) → review+
Comment on attachment 8514240 [details] [diff] [review]
[03] Bug 1091577: Use general-purpose notification runnable in Bluetooth daemon backend

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

::: dom/bluetooth/bluedroid/BluetoothDaemonHelpers.h
@@ +890,5 @@
> +    uint8_t service, opcode;
> +    uint16_t payloadSize;
> +    mPDU->GetHeader(service, opcode, payloadSize);
> +
> +    BT_LOGR("Unpacked PDU of type (%x,%x) still contains %zu Bytes of data.",

Maybe use BT_LOGD here?
Attachment #8514240 - Flags: review?(shuang) → review+
(In reply to Shawn Huang [:shawnjohnjr] from comment #10)
> Comment on attachment 8514240 [details] [diff] [review]
> [03] Bug 1091577: Use general-purpose notification runnable in Bluetooth
> daemon backend
> 
> Review of attachment 8514240 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bluetooth/bluedroid/BluetoothDaemonHelpers.h
> @@ +890,5 @@
> > +    uint8_t service, opcode;
> > +    uint16_t payloadSize;
> > +    mPDU->GetHeader(service, opcode, payloadSize);
> > +
> > +    BT_LOGR("Unpacked PDU of type (%x,%x) still contains %zu Bytes of data.",
> 
> Maybe use BT_LOGD here?

If there's unused data at the end of a PDU, it's very likely because of a violation of the HAL protocol. I'd like to keep BT_LOGR here, because I think that's significant enough to be reported in release builds.
Comment on attachment 8514241 [details] [diff] [review]
[04] Bug 1091577: Use general-purpose result runnable in Bluetooth daemon backend

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

::: dom/bluetooth/bluedroid/BluetoothDaemonSocketInterface.cpp
@@ +117,3 @@
>        IntStringIntResultRunnable::Dispatch(
>          GetResultHandler(), &BluetoothSocketResultHandler::Accept,
> +        ConstantInitOp3<int, nsString, int>(GetClientFd(), address,

Is there any concern to directly pass GetBdAddress()?
Attachment #8514241 - Flags: review?(shuang)
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #11)
> (In reply to Shawn Huang [:shawnjohnjr] from comment #10)
> > Comment on attachment 8514240 [details] [diff] [review]
> > [03] Bug 1091577: Use general-purpose notification runnable in Bluetooth
> > daemon backend
> > 
> > Review of attachment 8514240 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/bluetooth/bluedroid/BluetoothDaemonHelpers.h
> > @@ +890,5 @@
> > > +    uint8_t service, opcode;
> > > +    uint16_t payloadSize;
> > > +    mPDU->GetHeader(service, opcode, payloadSize);
> > > +
> > > +    BT_LOGR("Unpacked PDU of type (%x,%x) still contains %zu Bytes of data.",
> > 
> > Maybe use BT_LOGD here?
> 
> If there's unused data at the end of a PDU, it's very likely because of a
> violation of the HAL protocol. I'd like to keep BT_LOGR here, because I
> think that's significant enough to be reported in release builds.
Got it, thanks.
Created attachment 8516664 [details] [diff] [review]
[01] Bug 1091577: Added |BluetoothDaemonPDU::GetHeader| (v2)

Changes since v1:

  - fix sizes of memcpy'ed values
Attachment #8514238 - Attachment is obsolete: true
Attachment #8514238 - Flags: feedback?(btian)
Attachment #8516664 - Flags: review?(shuang)
Attachment #8516664 - Flags: feedback?(btian)
Comment on attachment 8516664 [details] [diff] [review]
[01] Bug 1091577: Added |BluetoothDaemonPDU::GetHeader| (v2)

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

Thanks.
Attachment #8516664 - Flags: review?(shuang) → review+
(In reply to Shawn Huang [:shawnjohnjr] from comment #12)
> Comment on attachment 8514241 [details] [diff] [review]
> [04] Bug 1091577: Use general-purpose result runnable in Bluetooth daemon
> backend
> 
> Review of attachment 8514241 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bluetooth/bluedroid/BluetoothDaemonSocketInterface.cpp
> @@ +117,3 @@
> >        IntStringIntResultRunnable::Dispatch(
> >          GetResultHandler(), &BluetoothSocketResultHandler::Accept,
> > +        ConstantInitOp3<int, nsString, int>(GetClientFd(), address,
> 
> Is there any concern to directly pass GetBdAddress()?

Ah, thanks for noticing. I'll update the code. I think in an earlier version if the patch, the arguments of ConstantInitOp3 where non-const, which made C++ require an actually constructed instance.
Created attachment 8516669 [details] [diff] [review]
[04] Bug 1091577: Use general-purpose result runnable in Bluetooth daemon backend (v2)

Changes since v1:

  - don't require constructed address-string object in |AcceptWatcher::Proceed|
Attachment #8514241 - Attachment is obsolete: true
Attachment #8514241 - Flags: feedback?(btian)
Attachment #8516669 - Flags: review?(shuang)
Attachment #8516669 - Flags: feedback?(btian)
Comment on attachment 8514241 [details] [diff] [review]
[04] Bug 1091577: Use general-purpose result runnable in Bluetooth daemon backend

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

::: dom/bluetooth/bluedroid/BluetoothDaemonSocketInterface.h
@@ +70,4 @@
>      ErrorRunnable;
>  
> +  typedef BluetoothResultRunnable3<BluetoothSocketResultHandler, void,
> +                                   int, nsString, int,

|const| is been removed. I guess this is what you mentioned build break.
BluetoothInterfaceHelpers.h:742:12: note:   no known conversion for argument 2 from 'const nsString' to 'nsString&'
Attachment #8514241 - Attachment is obsolete: false
Comment on attachment 8514241 [details] [diff] [review]
[04] Bug 1091577: Use general-purpose result runnable in Bluetooth daemon backend

Ah! air collision.
Attachment #8514241 - Attachment is obsolete: true
Attachment #8516669 - Flags: review?(shuang) → review+
Attachment #8514239 - Flags: feedback?(btian)
Attachment #8514240 - Flags: feedback?(btian)
Attachment #8514242 - Flags: feedback?(btian)
Attachment #8516664 - Flags: feedback?(btian)
Attachment #8516669 - Flags: feedback?(btian)
Blocks: 1065336
You need to log in before you can comment on or make changes to this bug.