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

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

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

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?

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

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

Changes since v1: - fix sizes of memcpy'ed values

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

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

Changes since v1: - don't require constructed address-string object in |AcceptWatcher::Proceed|

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 on attachment 8514241 [details] [diff] [review] [04] Bug 1091577: Use general-purpose result runnable in Bluetooth daemon backend Ah! air collision.

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

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