[Bluetooth] Prepare IPC classes for BT daemon

RESOLVED FIXED in 2.1 S7 (24Oct)

Status

Firefox OS
Bluetooth
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: tzimmermann, Assigned: tzimmermann)

Tracking

unspecified
2.1 S7 (24Oct)
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

To support the new BT daemon, we need to add some improvements to the socket IPC classes.
Created attachment 8505406 [details] [diff] [review]
[01] Bug 1083092: Introduce |mozilla::ipc::SocketBase|
Attachment #8505406 - Flags: review?(kyle)
Attachment #8505406 - Flags: feedback?(btian)
Created attachment 8505407 [details] [diff] [review]
[02] Bug 1083092: Data parameter for |mozilla::ipc::SocketIOSendTask| template
Attachment #8505407 - Flags: review?(kyle)
Attachment #8505407 - Flags: feedback?(btian)
Comment on attachment 8505406 [details] [diff] [review]
[01] Bug 1083092: Introduce |mozilla::ipc::SocketBase|

Everyone's on PTO...
Attachment #8505406 - Flags: review?(shuang)
Attachment #8505406 - Flags: review?(kyle)
Attachment #8505406 - Flags: feedback?(btian)
Attachment #8505407 - Flags: review?(shuang)
Attachment #8505407 - Flags: review?(kyle)
Attachment #8505407 - Flags: feedback?(btian)
Created attachment 8505409 [details] [diff] [review]
[03] Bug 1083092: Add |mozilla::ipc::UnixSocketIOBuffer|
Attachment #8505409 - Flags: review?(shuang)
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.
Attachment #8505406 - Flags: review?(shuang) → review+
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.
Attachment #8505407 - Flags: review?(shuang) → review+
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|
Attachment #8505409 - Flags: review?(shuang)
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'?
Flags: needinfo?(shuang)
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.
Flags: needinfo?(shuang)
Attachment #8505409 - Flags: review+
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
https://hg.mozilla.org/mozilla-central/rev/6fe61837183d
https://hg.mozilla.org/mozilla-central/rev/8a3c29ab6c2c
https://hg.mozilla.org/mozilla-central/rev/c8edfa5e934c
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S7 (24Oct)
https://hg.mozilla.org/mozilla-central/rev/6fe61837183d
https://hg.mozilla.org/mozilla-central/rev/8a3c29ab6c2c
https://hg.mozilla.org/mozilla-central/rev/c8edfa5e934c
You need to log in before you can comment on or make changes to this bug.