1083092 - [Bluetooth] Prepare IPC classes for BT daemon

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

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

To support the new BT daemon, we need to add some improvements to the socket IPC classes.

Comment 1

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

Attachment: [01] Bug 1083092: Introduce |mozilla::ipc::SocketBase|

Comment 2

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

Attachment: [02] Bug 1083092: Data parameter for |mozilla::ipc::SocketIOSendTask| template

Comment 3

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

Comment on attachment 8505406 [details] [diff] [review]
[01] Bug 1083092: Introduce |mozilla::ipc::SocketBase|

Everyone's on PTO...

Comment 4

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

Attachment: [03] Bug 1083092: Add |mozilla::ipc::UnixSocketIOBuffer|

Comment 5

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

Comment on attachment 8505406 [details] [diff] [review]
[01] Bug 1083092: Introduce |mozilla::ipc::SocketBase|

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

This looks fine to me.

Comment 6

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

Comment on attachment 8505407 [details] [diff] [review]
[02] Bug 1083092: Data parameter for |mozilla::ipc::SocketIOSendTask| template

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

The change makes sense. I'm fine with this change, although I'm not the best person to review UnixSocket.

Comment 7

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

Comment on attachment 8505409 [details] [diff] [review]
[03] Bug 1083092: Add |mozilla::ipc::UnixSocketIOBuffer|

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

::: ipc/unixsocket/SocketBase.cpp
@@ +57,5 @@
> +  const uint8_t* data = Consume(aLen);
> +  if (!data) {
> +    return NS_ERROR_OUT_OF_MEMORY;
> +  }
> +  memcpy(aValue, data, aLen);

I will be better to do sanity check for| memcpy|

@@ +67,5 @@
> +{
> +  if (((mAvailableSpace - mSize) < aLen)) {
> +    size_t availableSpace = mAvailableSpace + std::max(mAvailableSpace, aLen);
> +    uint8_t* data = new uint8_t[availableSpace];
> +    memcpy(data, mData, mSize);

I will be better to do sanity check for| memcpy|

Comment 8

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

Hi,

thanks for your review.

(In reply to Shawn Huang [:shawnjohnjr] from comment #7)
> Comment on attachment 8505409 [details] [diff] [review]
> [03] Bug 1083092: Add |mozilla::ipc::UnixSocketIOBuffer|
> 
> Review of attachment 8505409 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: ipc/unixsocket/SocketBase.cpp
> @@ +57,5 @@
> > +  const uint8_t* data = Consume(aLen);
> > +  if (!data) {
> > +    return NS_ERROR_OUT_OF_MEMORY;
> > +  }
> > +  memcpy(aValue, data, aLen);
> 
> I will be better to do sanity check for| memcpy|

What do you mean by 'sanity check'?

Comment 9

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

I was thinking if we need to check dest buffer size is big enough. But I was wrong, we don't need to do it.

Comment 10

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

Thank you, Shawn.

https://hg.mozilla.org/integration/b2g-inbound/rev/6fe61837183d
https://hg.mozilla.org/integration/b2g-inbound/rev/8a3c29ab6c2c
https://hg.mozilla.org/integration/b2g-inbound/rev/c8edfa5e934c

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6fe61837183d
https://hg.mozilla.org/mozilla-central/rev/8a3c29ab6c2c
https://hg.mozilla.org/mozilla-central/rev/c8edfa5e934c

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6fe61837183d
https://hg.mozilla.org/mozilla-central/rev/8a3c29ab6c2c
https://hg.mozilla.org/mozilla-central/rev/c8edfa5e934c