1091577 - [Bluetooth] Implement general-purpose runnables for Bluetooth backends

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Thomas Zimmermann [:tzimmermann] [:tdz]]

The notification and interface runnables for the Bluetooth backends need a rework. The current implementation is backend-specific and not always easily discoverable.

Comment 1

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 1091577: Added |BluetoothDaemonPDU::GetHeader|

Comment 2

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 1091577: Added general-purpose notification runnables for Bluetooth

Comment 3

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [03] Bug 1091577: Use general-purpose notification runnable in Bluetooth daemon backend

Comment 4

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [04] Bug 1091577: Use general-purpose result runnable in Bluetooth daemon backend

Comment 5

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [05] Bug 1091577: Remove unused helpers from daemon backend

Comment 6

[Thomas Zimmermann [:tzimmermann] [:tdz]]

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.

Comment 7

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

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?

Comment 8

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(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 9

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

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.

Comment 10

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

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?

Comment 11

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(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 12

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

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()?

Comment 13

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(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.

Comment 14

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 1091577: Added |BluetoothDaemonPDU::GetHeader| (v2)

Changes since v1:

  - fix sizes of memcpy'ed values

Comment 15

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Comment on attachment 8516664 [details] [diff] [review]
[01] Bug 1091577: Added |BluetoothDaemonPDU::GetHeader| (v2)

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

Thanks.

Comment 16

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(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.

Comment 17

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [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|

Comment 18

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

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&'

Comment 19

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Comment on attachment 8514241 [details] [diff] [review]
[04] Bug 1091577: Use general-purpose result runnable in Bluetooth daemon backend

Ah! air collision.

Comment 20

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Thank you!

https://hg.mozilla.org/integration/b2g-inbound/rev/2c43ea1d5a36
https://hg.mozilla.org/integration/b2g-inbound/rev/f01b382d95d7
https://hg.mozilla.org/integration/b2g-inbound/rev/9a0d1215a510
https://hg.mozilla.org/integration/b2g-inbound/rev/73a2073fea64
https://hg.mozilla.org/integration/b2g-inbound/rev/0f85799c04f4

https://treeherder.mozilla.org/ui/#/jobs?repo=b2g-inbound&revision=0f85799c04f4

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2c43ea1d5a36
https://hg.mozilla.org/mozilla-central/rev/f01b382d95d7
https://hg.mozilla.org/mozilla-central/rev/9a0d1215a510
https://hg.mozilla.org/mozilla-central/rev/73a2073fea64
https://hg.mozilla.org/mozilla-central/rev/0f85799c04f4