Reliably establish connection between Gecko and bluetoothd

RESOLVED FIXED in Firefox 38, Firefox OS v2.2

Status

Firefox OS
Bluetooth
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: tzimmermann, Assigned: tzimmermann)

Tracking

unspecified
2.2 S4 (23jan)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

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

Details

Attachments

(8 attachments, 5 obsolete attachments)

2.13 KB, patch
qdot
: review+
Details | Diff | Splinter Review
6.50 KB, patch
qdot
: review+
Details | Diff | Splinter Review
1.39 KB, patch
Ben Tian (inactive)
: review+
Details | Diff | Splinter Review
418 bytes, text/html
shawnjohnjr
: review+
Details
373 bytes, text/html
mwu
: review+
Details
379 bytes, text/html
mwu
: review+
Details
22.55 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
7.25 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
The current code for establishing connections between Gecko and bluetoothd contains race conditions and incompatible with BlueZ. This has to be fixed before we fully enable bluetoothd.
blocking-b2g: --- → 2.2?

Comment 1

3 years ago
Hi Thomas, could you help to provide ETA on this? thank you!
blocking-b2g: 2.2? → ---
feature-b2g: --- → 2.2+
Flags: needinfo?(tzimmermann)
Hi

(In reply to howie [:howie] from comment #1)
> Hi Thomas, could you help to provide ETA on this? thank you!

I'm actively working on this and have the basic code ready. This still needs several cleanups and reviews. I expect it to be ready by the end of next week (~Jan 16).
Flags: needinfo?(tzimmermann)

Comment 3

3 years ago
Thank you very much Thomas.
Target Milestone: --- → 2.2 S4 (23jan)
Created attachment 8547541 [details] [diff] [review]
[01] Bug 1119746: Signal active listening in |ListenSocket|
Attachment #8547541 - Flags: review?(kyle)
Created attachment 8547542 [details] [diff] [review]
[02] Bug 1119746: Support |ListenSocket| in |BluetoothDaemonConnection|
Attachment #8547542 - Flags: review?(kyle)
Created attachment 8547543 [details] [diff] [review]
[03] Bug 1119746: Fix ref-counting of bluetoothd channels
Attachment #8547543 - Flags: review?(btian)
Created attachment 8547545 [details] [diff] [review]
[04] Bug 1119746: Listen for socket connections when starting Bluetooth
Attachment #8547545 - Flags: review?(btian)
Created attachment 8547546 [details] [diff] [review]
[05] Bug 1119746: Support random postfix for Bluetooth daemon socket name
Attachment #8547546 - Flags: review?(btian)
Created attachment 8547548 [details]
Github tree for bluetoothd
Attachment #8547548 - Flags: review?(shuang)
Created attachment 8547552 [details]
Github pull request for gonk-misc

FYI: The Bluetooth init script is part of the bluetoothd pull request.
Attachment #8547552 - Flags: review?(mwu)
Created attachment 8547555 [details]
Github pull request for device-flame
Attachment #8547555 - Flags: review?(mwu)
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #10)
> Created attachment 8547552 [details]
> Github pull request for gonk-misc
> 
> FYI: The Bluetooth init script is part of the bluetoothd pull request.

I should mention that BlueZ 5 comes with it's own copy of init.bluetooth.rc. By adopting the same name, we'll be able to replace Bluedroid by BlueZ 5 easily.
Blocks: 1095491
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #12)
> (In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #10)
> > Created attachment 8547552 [details]
> > Github pull request for gonk-misc
> > 
> > FYI: The Bluetooth init script is part of the bluetoothd pull request.
> 
> I should mention that BlueZ 5 comes with it's own copy of init.bluetooth.rc.
> By adopting the same name, we'll be able to replace Bluedroid by BlueZ 5
> easily.

Does BlueZ 5 have a API layer similar to bluedroid now too? I thought the whole idea behind bluedroid was to drop dbus, but BlueZ couldn't do that due to GPL?
(In reply to Kyle Machulis [:kmachulis] [:qdot] (USE NEEDINFO?) from comment #13)
> (In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #12)
> > (In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #10)
> > > Created attachment 8547552 [details]
> > > Github pull request for gonk-misc
> > > 
> > > FYI: The Bluetooth init script is part of the bluetoothd pull request.
> > 
> > I should mention that BlueZ 5 comes with it's own copy of init.bluetooth.rc.
> > By adopting the same name, we'll be able to replace Bluedroid by BlueZ 5
> > easily.
> 
> Does BlueZ 5 have a API layer similar to bluedroid now too? I thought the
> whole idea behind bluedroid was to drop dbus, but BlueZ couldn't do that due
> to GPL?

The BlueZ project wrote a Bluedroid driver that talks to the BlueZ 5 daemon. We use the same protocol for talking to our daemon. But it all happens at the raw socket level, no DBus is involved. More infos are at [1] and the protocol is at [2].

[1] https://lwn.net/Articles/597293/
[2] https://git.kernel.org/cgit/bluetooth/bluez.git/tree/android/hal-ipc-api.txt
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #14)
> The BlueZ project wrote a Bluedroid driver that talks to the BlueZ 5 daemon.
> We use the same protocol for talking to our daemon. But it all happens at
> the raw socket level, no DBus is involved. More infos are at [1] and the
> protocol is at [2].

Does this mean linux desktop bluetooth should now "just work" if I have a kernel with BlueZ 5?
(In reply to Kyle Machulis [:kmachulis] [:qdot] (USE NEEDINFO?) from comment #15)
> (In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #14)
> > The BlueZ project wrote a Bluedroid driver that talks to the BlueZ 5 daemon.
> > We use the same protocol for talking to our daemon. But it all happens at
> > the raw socket level, no DBus is involved. More infos are at [1] and the
> > protocol is at [2].
> 
> Does this mean linux desktop bluetooth should now "just work" if I have a
> kernel with BlueZ 5?

No, Desktop still uses DBus.

Comment 17

3 years ago
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #12)
> I should mention that BlueZ 5 comes with it's own copy of init.bluetooth.rc.
> By adopting the same name, we'll be able to replace Bluedroid by BlueZ 5
> easily.

Is it much different than our copy? I'm more inclined to just directly inline the thing rather than add a new init.rc import.

Comment 18

3 years ago
Comment on attachment 8547543 [details] [diff] [review]
[03] Bug 1119746: Fix ref-counting of bluetoothd channels

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

LGTM.
Attachment #8547543 - Flags: review?(btian) → review+

Comment 19

3 years ago
Comment on attachment 8547545 [details] [diff] [review]
[04] Bug 1119746: Listen for socket connections when starting Bluetooth

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

Please see my question below. Also in patch description I think "we" should be "when" as following.

  Currently, Gecko connects to a running instance of bluetoothd "when" it
  starts the daemon backend. ...

::: dom/bluetooth/bluedroid/BluetoothDaemonInterface.cpp
@@ +1947,5 @@
> +      BT_WARNING("Address too long for socket struct!");
> +      return false;
> +    }
> +
> +    memset(aAddr.un.sun_path, '\0', sNameOffset); // abstract socket

Why is '\0' required at the beginning of |sun_path|?

@@ +1994,5 @@
> + *
> + *  (2) Start the Bluetooth daemon: When the daemon starts up it will
> + *      open two socket connections to Gecko and thus create the command
> + *      and notification channels. Gecko already opened the listen socket
> + *      in step (1). Step (2) end with the creation of the command channel.

nit: end's'

@@ +2000,5 @@
> + *  (3) Start listening for the notification channel's socket connection:
> + *      At the end of step (2), the command channel was opened by the
> + *      daemon. In step (3), the daemon immediately tries to open the next
> + *      socket for the notification channel. Gecko will now accept an
> + *      incoming connection requests for this socket. The listen socket

I think 'an' is redundant, right?

... accept incoming connection requests for this socket.
Attachment #8547545 - Flags: review?(btian)

Comment 20

3 years ago
Comment on attachment 8547546 [details] [diff] [review]
[05] Bug 1119746: Support random postfix for Bluetooth daemon socket name

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

r=me with comment addressed.

::: dom/bluetooth/bluedroid/BluetoothDaemonInterface.cpp
@@ +2096,5 @@
> +  // The listen socket's name is generated with a random postfix. This
> +  // avoids naming collisions if we still have a listen socket from a
> +  // previously failed cleanup. It also makes it hard for malicious
> +  // external programs to capture the socket name or connect before
> +  // the daemon can do so. If no random prefix can be generated, we

random 'postfix'
Attachment #8547546 - Flags: review?(btian) → review+
(In reply to Michael Wu [:mwu] from comment #17)
> (In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #12)
> > I should mention that BlueZ 5 comes with it's own copy of init.bluetooth.rc.
> > By adopting the same name, we'll be able to replace Bluedroid by BlueZ 5
> > easily.
> 
> Is it much different than our copy? I'm more inclined to just directly
> inline the thing rather than add a new init.rc import.

Yes, please have a look at [1]. BlueZ uses a fixed socket name, but I added the ability to configure the socket name as parameter when the daemon gets started. That makes the whole thing more robust, but required changes to the init script.

BTW Why do yo not what to import additional init scripts? Currently, we duplicate several rules for NFC and RIL in init.b2g.rc and init.target.rc. Having those rules in a shared script would seem like an improvement to me.

[1] https://git.kernel.org/cgit/bluetooth/bluez.git/tree/android/init.bluetooth.rc
Comment on attachment 8547548 [details]
Github tree for bluetoothd

I have left some comments on PR. Overall is good. Can you answer my question? Thanks.
Comment on attachment 8547545 [details] [diff] [review]
[04] Bug 1119746: Listen for socket connections when starting Bluetooth

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

::: dom/bluetooth/bluedroid/BluetoothDaemonInterface.cpp
@@ +1868,5 @@
>        // Close command channel
>        mCmdChannel->CloseSocket();
> +    case CMD_CHANNEL:
> +      // Stop daemon and close listen socket
> +      NS_WARN_IF(property_set("ctl.stop", "bluetoothd"));

This may cause compiling error.
../../../../../../../code/b2g37_v2_2/mozilla-b2g37_v2_2/dom/bluetooth/bluedroid/BluetoothDaemonInterface.cpp:1877:53: error: value computed is not used [-Werror=unused-value]
../../../../../../../code/b2g37_v2_2/mozilla-b2g37_v2_2/dom/bluetooth/bluedroid/BluetoothDaemonInterface.cpp: In member function 'virtual void mozilla::dom::bluetooth::BluetoothDaemonInterface::Init(mozilla::dom::bluetooth::BluetoothNotificationHandler*, mozilla::dom::bluetooth::BluetoothResultHandler*)':

@@ +2022,5 @@
> +  static const char BASE_SOCKET_NAME[] = "bluetoothd";
> +
> +  // If we could not cleanup before and an old instance of the
> +  // daemon is still running, we kill it here.
> +  NS_WARN_IF(property_set("ctl.stop", "bluetoothd"));

../../../../../../../code/b2g37_v2_2/mozilla-b2g37_v2_2/dom/bluetooth/bluedroid/BluetoothDaemonInterface.cpp:2072:49: error: value computed is not used [-Werror=unused-value]
cc1plus: all warnings being treated as errors
(In reply to Shawn Huang [:shawnjohnjr] from comment #23)
> Comment on attachment 8547545 [details] [diff] [review]
> [04] Bug 1119746: Listen for socket connections when starting Bluetooth
> 
> Review of attachment 8547545 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bluetooth/bluedroid/BluetoothDaemonInterface.cpp
> @@ +1868,5 @@
> >        // Close command channel
> >        mCmdChannel->CloseSocket();
> > +    case CMD_CHANNEL:
> > +      // Stop daemon and close listen socket
> > +      NS_WARN_IF(property_set("ctl.stop", "bluetoothd"));
> 
> This may cause compiling error.
> ../../../../../../../code/b2g37_v2_2/mozilla-b2g37_v2_2/dom/bluetooth/
> bluedroid/BluetoothDaemonInterface.cpp:1877:53: error: value computed is not
> used [-Werror=unused-value]
> ../../../../../../../code/b2g37_v2_2/mozilla-b2g37_v2_2/dom/bluetooth/
> bluedroid/BluetoothDaemonInterface.cpp: In member function 'virtual void
> mozilla::dom::bluetooth::BluetoothDaemonInterface::Init(mozilla::dom::
> bluetooth::BluetoothNotificationHandler*,
> mozilla::dom::bluetooth::BluetoothResultHandler*)':
> 
> @@ +2022,5 @@
> > +  static const char BASE_SOCKET_NAME[] = "bluetoothd";
> > +
> > +  // If we could not cleanup before and an old instance of the
> > +  // daemon is still running, we kill it here.
> > +  NS_WARN_IF(property_set("ctl.stop", "bluetoothd"));
> 
> ../../../../../../../code/b2g37_v2_2/mozilla-b2g37_v2_2/dom/bluetooth/
> bluedroid/BluetoothDaemonInterface.cpp:2072:49: error: value computed is not
> used [-Werror=unused-value]
> cc1plus: all warnings being treated as errors

Thanks for testing. Which platform is that?
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #24)
> (In reply to Shawn Huang [:shawnjohnjr] from comment #23)
> > Comment on attachment 8547545 [details] [diff] [review]
> > [04] Bug 1119746: Listen for socket connections when starting Bluetooth
> > 
> > Review of attachment 8547545 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/bluetooth/bluedroid/BluetoothDaemonInterface.cpp
> > @@ +1868,5 @@
> > >        // Close command channel
> > >        mCmdChannel->CloseSocket();
> > > +    case CMD_CHANNEL:
> > > +      // Stop daemon and close listen socket
> > > +      NS_WARN_IF(property_set("ctl.stop", "bluetoothd"));
> > 
> > This may cause compiling error.
> > ../../../../../../../code/b2g37_v2_2/mozilla-b2g37_v2_2/dom/bluetooth/
> > bluedroid/BluetoothDaemonInterface.cpp:1877:53: error: value computed is not
> > used [-Werror=unused-value]
> > ../../../../../../../code/b2g37_v2_2/mozilla-b2g37_v2_2/dom/bluetooth/
> > bluedroid/BluetoothDaemonInterface.cpp: In member function 'virtual void
> > mozilla::dom::bluetooth::BluetoothDaemonInterface::Init(mozilla::dom::
> > bluetooth::BluetoothNotificationHandler*,
> > mozilla::dom::bluetooth::BluetoothResultHandler*)':
> > 
> > @@ +2022,5 @@
> > > +  static const char BASE_SOCKET_NAME[] = "bluetoothd";
> > > +
> > > +  // If we could not cleanup before and an old instance of the
> > > +  // daemon is still running, we kill it here.
> > > +  NS_WARN_IF(property_set("ctl.stop", "bluetoothd"));
> > 
> > ../../../../../../../code/b2g37_v2_2/mozilla-b2g37_v2_2/dom/bluetooth/
> > bluedroid/BluetoothDaemonInterface.cpp:2072:49: error: value computed is not
> > used [-Werror=unused-value]
> > cc1plus: all warnings being treated as errors
> 
> Thanks for testing. Which platform is that?

I compiled on flame-kk, maybe it's because gecko enabled '-Werror'.
(In reply to Shawn Huang [:shawnjohnjr] from comment #25)
> (In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #24)
> > (In reply to Shawn Huang [:shawnjohnjr] from comment #23)
> 
> I compiled on flame-kk, maybe it's because gecko enabled '-Werror'.
See bug 1073003.
(In reply to Shawn Huang [:shawnjohnjr] from comment #25)
> (In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #24)
> > (In reply to Shawn Huang [:shawnjohnjr] from comment #23)
> > > Comment on attachment 8547545 [details] [diff] [review]
> > > [04] Bug 1119746: Listen for socket connections when starting Bluetooth
> > > 
> > > Review of attachment 8547545 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > ::: dom/bluetooth/bluedroid/BluetoothDaemonInterface.cpp
> > > @@ +1868,5 @@
> > > >        // Close command channel
> > > >        mCmdChannel->CloseSocket();
> > > > +    case CMD_CHANNEL:
> > > > +      // Stop daemon and close listen socket
> > > > +      NS_WARN_IF(property_set("ctl.stop", "bluetoothd"));
> > > 
> > > This may cause compiling error.
> > > ../../../../../../../code/b2g37_v2_2/mozilla-b2g37_v2_2/dom/bluetooth/
> > > bluedroid/BluetoothDaemonInterface.cpp:1877:53: error: value computed is not
> > > used [-Werror=unused-value]
> > > ../../../../../../../code/b2g37_v2_2/mozilla-b2g37_v2_2/dom/bluetooth/
> > > bluedroid/BluetoothDaemonInterface.cpp: In member function 'virtual void
> > > mozilla::dom::bluetooth::BluetoothDaemonInterface::Init(mozilla::dom::
> > > bluetooth::BluetoothNotificationHandler*,
> > > mozilla::dom::bluetooth::BluetoothResultHandler*)':
> > > 
> > > @@ +2022,5 @@
> > > > +  static const char BASE_SOCKET_NAME[] = "bluetoothd";
> > > > +
> > > > +  // If we could not cleanup before and an old instance of the
> > > > +  // daemon is still running, we kill it here.
> > > > +  NS_WARN_IF(property_set("ctl.stop", "bluetoothd"));
> > > 
> > > ../../../../../../../code/b2g37_v2_2/mozilla-b2g37_v2_2/dom/bluetooth/
> > > bluedroid/BluetoothDaemonInterface.cpp:2072:49: error: value computed is not
> > > used [-Werror=unused-value]
> > > cc1plus: all warnings being treated as errors
> > 
> > Thanks for testing. Which platform is that?
> 
> I compiled on flame-kk, maybe it's because gecko enabled '-Werror'.

I guess I should update. :/ flame-kk always built for me.
Comment on attachment 8547548 [details]
Github tree for bluetoothd

Updated github pull request:

  - removed out-commented code
  - print custom error for unknown command-line options
  - removed unnecessary triggers from init script
One thing I don't understand, i saw there are two places (init.target.rc/init.b2g.rc) actually import "/init.bluetooth.rc". Why do we need to import twice?
Comment on attachment 8547546 [details] [diff] [review]
[05] Bug 1119746: Support random postfix for Bluetooth daemon socket name

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

::: dom/bluetooth/bluedroid/BluetoothDaemonInterface.cpp
@@ +1827,5 @@
> +    case LISTEN_SOCKET: {
> +        // Init, step 2: Start Bluetooth daemon */
> +        nsCString value("bluetoothd:-a ");
> +        value.Append(mListenSocketName);
> +        if (NS_WARN_IF(property_set("ctl.start", value.get()) < 0)) {

I'm a little bit curious here, can "ctl.start" take parameters? I thought ctl.start only takes 'service name' which mentioned in init.bluetooth.rc.
Attachment #8547548 - Flags: review?(shuang) → review+
(In reply to Shawn Huang [:shawnjohnjr] from comment #30)
> Comment on attachment 8547546 [details] [diff] [review]
> [05] Bug 1119746: Support random postfix for Bluetooth daemon socket name
> 
> Review of attachment 8547546 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bluetooth/bluedroid/BluetoothDaemonInterface.cpp
> @@ +1827,5 @@
> I'm a little bit curious here, can "ctl.start" take parameters? I thought
> ctl.start only takes 'service name' which mentioned in init.bluetooth.rc.
Opps, looks like I asked a silly question. I was wrong about ctl.start. :( It looks like parameters indeed pass to bluetoothd.
(In reply to Shawn Huang [:shawnjohnjr] from comment #30)
> Comment on attachment 8547546 [details] [diff] [review]
> [05] Bug 1119746: Support random postfix for Bluetooth daemon socket name
> 
> Review of attachment 8547546 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bluetooth/bluedroid/BluetoothDaemonInterface.cpp
> @@ +1827,5 @@
> > +    case LISTEN_SOCKET: {
> > +        // Init, step 2: Start Bluetooth daemon */
> > +        nsCString value("bluetoothd:-a ");
> > +        value.Append(mListenSocketName);
> > +        if (NS_WARN_IF(property_set("ctl.start", value.get()) < 0)) {
> 
> I'm a little bit curious here, can "ctl.start" take parameters? I thought
> ctl.start only takes 'service name' which mentioned in init.bluetooth.rc.

Init has some extra magic for ctl.{start,stop,restart}. Part of that is to allow passing parameters for one-shot services.
(In reply to Shawn Huang [:shawnjohnjr] from comment #29)
> One thing I don't understand, i saw there are two places
> (init.target.rc/init.b2g.rc) actually import "/init.bluetooth.rc". Why do we
> need to import twice?

For some reason I don't know, Flame uses init.target.rc instead of init.b2g.rc, or something like that. It doesn't work on Flame if I don't include the Bluetooth script in init.target.rc.
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #33)
> (In reply to Shawn Huang [:shawnjohnjr] from comment #29)
> > One thing I don't understand, i saw there are two places
> > (init.target.rc/init.b2g.rc) actually import "/init.bluetooth.rc". Why do we
> > need to import twice?
> 
> For some reason I don't know, Flame uses init.target.rc instead of
> init.b2g.rc, or something like that. It doesn't work on Flame if I don't
> include the Bluetooth script in init.target.rc.

Got it. It looks like only qc platforms don't use init.b2g.rc but the rest of projects use init.b2g.rc file.
Hi

> 
> ::: dom/bluetooth/bluedroid/BluetoothDaemonInterface.cpp
> @@ +1947,5 @@
> > +      BT_WARNING("Address too long for socket struct!");
> > +      return false;
> > +    }
> > +
> > +    memset(aAddr.un.sun_path, '\0', sNameOffset); // abstract socket
> 
> Why is '\0' required at the beginning of |sun_path|?

Usually the names of Unix sockets are file paths. But we don't want the socket to show up in the file system; for reasons of resource cleanup and security.

Linux has the feature of 'abstract' socket names.  The extra '\0' at the beginning of the name signals to Linux that the name is just a string and not a file path. The man page has all the details. [1]

> 
> @@ +2000,5 @@
> > + *  (3) Start listening for the notification channel's socket connection:
> > + *      At the end of step (2), the command channel was opened by the
> > + *      daemon. In step (3), the daemon immediately tries to open the next
> > + *      socket for the notification channel. Gecko will now accept an
> > + *      incoming connection requests for this socket. The listen socket
> 
> I think 'an' is redundant, right?

There's an 's' at the end of request. And I'll rewrite the paragraph to make it easier to understand. Thanks for proof-reading all these comments.

[1] http://man7.org/linux/man-pages/man7/unix.7.html
Created attachment 8548774 [details] [diff] [review]
[04] Bug 1119746: Listen for socket connections when starting Bluetooth (v2)

Changes since v1:

  - fixed typos in comments and commit message
Attachment #8547545 - Attachment is obsolete: true
Attachment #8548774 - Flags: review?(btian)
Created attachment 8548776 [details] [diff] [review]
[05] Bug 1119746: Support random postfix for Bluetooth daemon socket name (v2)

Changes since v1:

  - fixed typos in comments
Attachment #8547546 - Attachment is obsolete: true
Attachment #8548776 - Flags: review+

Comment 38

3 years ago
Comment on attachment 8548774 [details] [diff] [review]
[04] Bug 1119746: Listen for socket connections when starting Bluetooth (v2)

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

r=me with nit addressed. Thanks for explanation in comment 35.

::: dom/bluetooth/bluedroid/BluetoothDaemonInterface.cpp
@@ +2000,5 @@
> + *  (3) Start listening for the notification channel's socket connection:
> + *      At the end of step (2), the command channel was opened by the
> + *      daemon. In step (3), the daemon immediately tries to open the
> + *      next socket for the notification channel. Gecko will accept the
> + *      incomming connection request for the notification channel. The

typo: incoming
Attachment #8548774 - Flags: review?(btian) → review+
(In reply to Ben Tian [:btian] from comment #38)
> Comment on attachment 8548774 [details] [diff] [review]
> [04] Bug 1119746: Listen for socket connections when starting Bluetooth (v2)
> 
> Review of attachment 8548774 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with nit addressed. Thanks for explanation in comment 35.
> 
> ::: dom/bluetooth/bluedroid/BluetoothDaemonInterface.cpp
> @@ +2000,5 @@
> > + *  (3) Start listening for the notification channel's socket connection:
> > + *      At the end of step (2), the command channel was opened by the
> > + *      daemon. In step (3), the daemon immediately tries to open the
> > + *      next socket for the notification channel. Gecko will accept the
> > + *      incomming connection request for the notification channel. The
> 
> typo: incoming

I made this mistake several times before. I don't know why I never get this right. :D
Created attachment 8548797 [details] [diff] [review]
[04] Bug 1119746: Listen for socket connections when starting Bluetooth (v3)

Changes since v2:

  - fixed another typo
Attachment #8548774 - Attachment is obsolete: true
Attachment #8548797 - Flags: review+
Created attachment 8548810 [details] [diff] [review]
[04] Bug 1119746: Listen for socket connections when starting Bluetooth (v4)

Changes since v3:

  - fix compiler error about unused return value

In the previous patch, I forgot about the compile-time error that Shawn reported. I updated, but it still don't see the error.
Attachment #8548797 - Attachment is obsolete: true
Attachment #8548810 - Flags: review+
Created attachment 8548813 [details] [diff] [review]
[05] Bug 1119746: Support random postfix for Bluetooth daemon socket name (v3)

Changes since v2:

  - rebased onto [04](v4)
Attachment #8548776 - Attachment is obsolete: true
Attachment #8548813 - Flags: review+
Blocks: 1122025
Blocks: 1065336

Updated

3 years ago
Attachment #8547552 - Flags: review?(mwu) → review+

Updated

3 years ago
Attachment #8547555 - Flags: review?(mwu) → review+
Great! mwu already r+ :)
Thomas, can you apply v2.2 approval, so we can uplift to v2.2 today. Thanks!
Flags: needinfo?(tzimmermann)
Comment on attachment 8547541 [details] [diff] [review]
[01] Bug 1119746: Signal active listening in |ListenSocket|

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 

We need this patch to establish connections between Gecko and bluetoothd, our Bluetooth daemon.

User impact if declined: 

No bluetoothd in v2.2

Testing completed: 

Tested locally on JB, KK platforms

Risk to taking this patch (and alternatives if risky): 

Low. The changes are self-contained in Bluetooth and can be rolled back easily.

String or UUID changes made by this patch:

None.
Attachment #8547541 - Flags: approval-mozilla-b2g37?
Comment on attachment 8547542 [details] [diff] [review]
[02] Bug 1119746: Support |ListenSocket| in |BluetoothDaemonConnection|

[Approval Request Comment]

Please see comment 45 for these information.

Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed: 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:
Attachment #8547542 - Flags: approval-mozilla-b2g37?
Comment on attachment 8547543 [details] [diff] [review]
[03] Bug 1119746: Fix ref-counting of bluetoothd channels

[Approval Request Comment]

Please see comment 45 for these information.

Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed: 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:
Attachment #8547543 - Flags: approval-mozilla-b2g37?
Comment on attachment 8548810 [details] [diff] [review]
[04] Bug 1119746: Listen for socket connections when starting Bluetooth (v4)

[Approval Request Comment]

Please see comment 45 for these information.

Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed: 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:
Attachment #8548810 - Flags: approval-mozilla-b2g37?
Comment on attachment 8548813 [details] [diff] [review]
[05] Bug 1119746: Support random postfix for Bluetooth daemon socket name (v3)

[Approval Request Comment]

Please see comment 45 for these information.

Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed: 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:
Attachment #8548813 - Flags: approval-mozilla-b2g37?
Try looks good, please check-in the patches. Thanks!
Keywords: checkin-needed
[Tracking Requested - why for this release]:
status-b2g-v2.2: --- → affected
Comment on attachment 8547548 [details]
Github tree for bluetoothd

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

Please see comment 45 for these information.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed: 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:
Attachment #8547548 - Flags: approval-mozilla-b2g37?
Comment on attachment 8547552 [details]
Github pull request for gonk-misc

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

Please see comment 45 for these information.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed: 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:
Attachment #8547552 - Flags: approval-mozilla-b2g37?
Comment on attachment 8547555 [details]
Github pull request for device-flame

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

Please see comment 45 for these information.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed: 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:
Attachment #8547555 - Flags: approval-mozilla-b2g37?
Eric, can you please help with some exploratory QA around this on master/central ?
Flags: needinfo?(echang)
Keywords: verifyme
(In reply to bhavana bajaj [:bajaj] from comment #57)
> Eric, can you please help with some exploratory QA around this on
> master/central ?

Hi bhavana,
QA cannot test because "Bug 1065336 - [Bluetooth] Add support for Bluedroid daemon to Gecko" has not been submitted, so bluetooth daemon is not been enabled.

Bug 1065336 will turn on bluetooth daemon feature and it has been blocked by the current bug (bug 119746). We need to land this patch set first.
Flags: needinfo?(bbajaj)
Comment on attachment 8547541 [details] [diff] [review]
[01] Bug 1119746: Signal active listening in |ListenSocket|

I would have preferred one roll-up patch for approval ease ;) may be something to keep in  mind next time approval is seeked :)
Flags: needinfo?(bbajaj)
Attachment #8547541 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+

Updated

3 years ago
Attachment #8547542 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+

Updated

3 years ago
Attachment #8547543 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+

Updated

3 years ago
Attachment #8547548 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+

Updated

3 years ago
Attachment #8547552 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+

Updated

3 years ago
Attachment #8547555 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+

Updated

3 years ago
Attachment #8548810 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+

Updated

3 years ago
Attachment #8548813 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Current status: BT L build is not yet ready, daemon not on, need to resolve the root access issue, I think we can test regression after most of the bugs are resolved. Will check with RD if we need specific tests on certain bugs.
Flags: needinfo?(echang)
Keywords: verifyme
status-b2g-master: --- → fixed
status-b2g-v2.2: affected → fixed
Blocks: 1126720
You need to log in before you can comment on or make changes to this bug.