Closed Bug 1249518 Opened 8 years ago Closed 8 years ago

Make DaemonSocketPDU able to send multiple file descriptors in single unit.

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: kechen, Assigned: JamesCheng)

Details

Attachments

(1 file, 7 obsolete files)

In current codebase, DaemonSocketPDU can only sends a file descriptor in single unit[1].

However, in some cases, we might need to transit structures that contain more than one file descriptors.


[1] https://dxr.mozilla.org/mozilla-central/source/ipc/hal/DaemonSocketPDU.cpp#149
Assignee: nobody → jacheng
I will start to work on this.
Hello Thomas,

We would like your feedback for the patch which is simply check the data for single/multiple fd received and store in an array.

Is it a reasonable modification for Kevin Chen's requirement?

Thank you~
Attachment #8721455 - Flags: feedback?(tzimmermann)
Comment on attachment 8721455 [details] [diff] [review]
Make-DaemonSocketPDU-able-to-send-multip.patch

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

::: ipc/hal/DaemonSocketPDU.cpp
@@ +141,5 @@
>    SetRange(0, res);
>  
>    struct cmsghdr *chdr = CMSG_FIRSTHDR(&msg);
>  
>    for (; chdr; chdr = CMSG_NXTHDR(&msg, chdr)) {

This for loop already walks over the fields of the ancillary data one-by-one. It will see each contained file descriptor. All you have to do is to append the value to 'mReceivedFds'. Please also refer to the respective documentation. [1]

[1] http://alas.matf.bg.ac.rs/manuals/lspe/snode=153.html

::: ipc/hal/DaemonSocketPDU.h
@@ +77,5 @@
> +  int AcquireFd(nsTArray<ScopedClose>::index_type aIndex = 0);
> +  nsTArray<ScopedClose>::size_type AcquireFdCount()
> +  {
> +    return mReceivedFds.Length();
> +  }

Maybe rather add |AcquireFds|, which returns the array of file descriptors.
Attachment #8721455 - Flags: feedback?(tzimmermann)
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #3)
> Comment on attachment 8721455 [details] [diff] [review]
> Make-DaemonSocketPDU-able-to-send-multip.patch
> 
> Review of attachment 8721455 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: ipc/hal/DaemonSocketPDU.cpp
> @@ +141,5 @@
> >    SetRange(0, res);
> >  
> >    struct cmsghdr *chdr = CMSG_FIRSTHDR(&msg);
> >  
> >    for (; chdr; chdr = CMSG_NXTHDR(&msg, chdr)) {
> 
> This for loop already walks over the fields of the ancillary data
> one-by-one. It will see each contained file descriptor. All you have to do
> is to append the value to 'mReceivedFds'. Please also refer to the
> respective documentation. [1]
> 
Hi Thomas,

I am not quite sure how should server side need to do when server wants to send multiple file descriptors.

I found that if I want to send two fds and write a sample server like below (server side)
int fd[2];// fd which should be sent
control_message = CMSG_FIRSTHDR(&message);
control_message->cmsg_level = SOL_SOCKET;
control_message->cmsg_type = SCM_RIGHTS;
control_message->cmsg_len = CMSG_LEN(sizeof(fd[0]) * 2); // double length.
int* dataBuff = ((int*)CMSG_DATA(control_message));
memcpy(dataBuff, fd, 2 * sizeof(int)); //write two fds into message
return sendmsg(sock, &message, 0);

And the client can receive the two fds as the patch.
// Retrieve sent file descriptors.
size_t fdCount = (chdr->cmsg_len - CMSG_ALIGN(sizeof(struct cmsghdr))) / sizeof(int);
for (size_t i = 0; i < fdCount; i++) {
  mReceivedFds.AppendElement(ScopedClose(*(static_cast<int*>(CMSG_DATA(chdr)) + i)));
}

I have tried to modified the server's code trying to make client 
|for (; chdr; chdr = CMSG_NXTHDR(&msg, chdr)) {|  runs more than once...but I failed.
The for loop always run once.

Could you please give me some hint or sample how to sned multiple fds in server side?

I take this link for reference(single fd) 
http://blog.varunajayasiri.com/passing-file-descriptors-between-processes-using-sendmsg-and-recvmsg

Thank you.


> [1] http://alas.matf.bg.ac.rs/manuals/lspe/snode=153.html
> 
> ::: ipc/hal/DaemonSocketPDU.h
> @@ +77,5 @@
> > +  int AcquireFd(nsTArray<ScopedClose>::index_type aIndex = 0);
> > +  nsTArray<ScopedClose>::size_type AcquireFdCount()
> > +  {
> > +    return mReceivedFds.Length();
> > +  }
> 
> Maybe rather add |AcquireFds|, which returns the array of file descriptors.

Thanks, 
nsTArray supports move constructor, 
so I can just |return mReceivedFds;| to keep the ScopedClose object not close the fd which it stores.
I will try this instead.
Flags: needinfo?(tzimmermann)
Hi

> I am not quite sure how should server side need to do when server wants to
> send multiple file descriptors.

The ancillary data consists of multiple headers, with each header transferring a single file descriptor. Your error is trying to send multiple file descriptors in a single header.

> I found that if I want to send two fds and write a sample server like below
> (server side)
> int fd[2];// fd which should be sent
> control_message = CMSG_FIRSTHDR(&message);
> control_message->cmsg_level = SOL_SOCKET;
> control_message->cmsg_type = SCM_RIGHTS;
> control_message->cmsg_len = CMSG_LEN(sizeof(fd[0]) * 2); // double length.

Here's the first problem.

> int* dataBuff = ((int*)CMSG_DATA(control_message));
> memcpy(dataBuff, fd, 2 * sizeof(int)); //write two fds into message

Here's the second problem.

It should work if you do this:

// message->msg_control[len] needs space for multiple headers!

control_message = CMSG_FIRSTHDR(&message); // start with first header

for (/* each int in fd[] */) {

  control_message->cmsg_level = SOL_SOCKET;
  control_message->cmsg_type = SCM_RIGHTS;
  control_message->cmsg_len = CMSG_LEN(sizeof(fd[i]));

  int* dataBuff = ((int*)CMSG_DATA(control_message));
  memcpy(dataBuff, fd + i, sizeof(fd[i]));

  control_message = CMSG_NEXTHDR(&message); // advance to next header for next iteration
}

And then the receiver will iterate over the individual headers and get file-descriptors one by one.
Flags: needinfo?(tzimmermann)
Hi Thomas,

I followed your suggestion and modified the patch.

Please take a look at the new revision patch.

And I would like to know that

my previous patch using |send multiple file descriptors in a single header| is a bad approach?

Instead, using single header that transfers a single file descriptor is recommended or a common pattern?

Thank you.
Attachment #8722426 - Flags: feedback?(tzimmermann)
Comment on attachment 8722426 [details] [diff] [review]
Make-DaemonSocketPDU-able-to-send-multip.patch

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

I don't know what you mean by 'recommended approach'; one file descriptor per header is simply how is works. Please read the documentation of this feature.

I'm not going to review further patches until there's working code for the TV daemon and Gecko.

::: dom/bluetooth/bluedroid/BluetoothDaemonSocketInterface.cpp
@@ +220,5 @@
>    operator () (int& aArg1) const
>    {
>      DaemonSocketPDU& pdu = GetPDU();
>  
> +    aArg1 = pdu.AcquireFds()[0];

The array might be empty. You have to check for that.

@@ +277,5 @@
>                                          DaemonSocketPDU& aPDU,
>                                          BluetoothSocketResultHandler* aRes)
>  {
>    /* the file descriptor is attached in the PDU's ancillary data */
> +  int fd = aPDU.AcquireFds()[0];

Same here.

::: ipc/hal/DaemonSocketPDU.cpp
@@ +147,5 @@
>        continue;
>      }
> +    // Retrieve sent file descriptors.
> +    size_t fdCount = (chdr->cmsg_len - CMSG_ALIGN(sizeof(struct cmsghdr))) / sizeof(int);
> +    for (size_t i = 0; i < fdCount; i++) {

You don't need this loop.
Attachment #8722426 - Flags: feedback?(tzimmermann) → feedback-
Refine patch as comment mentioned.
Attachment #8721455 - Attachment is obsolete: true
Attachment #8722426 - Attachment is obsolete: true
Comment on attachment 8722543 [details] [diff] [review]
Make-DaemonSocketPDU-able-to-send-multip.patch

Hi Thomas,
Kevin has just uploaded the patch on Bug 1229308 attachment 8724619 [details] [diff]

sending multiple fds with multiple headers.

Could you please review this patch for receiving multiple fds?

Thanks.
Attachment #8722543 - Flags: review?(tzimmermann)
Comment on attachment 8722543 [details] [diff] [review]
Make-DaemonSocketPDU-able-to-send-multip.patch

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

Hi,

A few nits should be fixed, but the patch is OK in general..

::: dom/bluetooth/bluedroid/BluetoothDaemonSocketInterface.cpp
@@ +226,5 @@
> +      aArg1 = receiveFds[0];
> +    } else {
> +      // Nothing received, assign the invalid fd
> +      aArg1 = -1;
> +    }

Use early-out:

if (!fds.length) {
  return error_code
}

arg1 = fd[0]

if (arg1 < 0)
  return error code;

@@ +287,5 @@
> +  auto receiveFds = aPDU.AcquireFds();
> +  int fd = -1;
> +  if (receiveFds.Length() > 0) {
> +    fd = receiveFds[0];
> +  }

Use early-out here and send an |ErrorRunnable|.

::: ipc/hal/DaemonSocketPDU.cpp
@@ +141,3 @@
>    SetRange(0, res);
>  
>    struct cmsghdr *chdr = CMSG_FIRSTHDR(&msg);

Put '*' next to type.

@@ +147,5 @@
>        continue;
>      }
> +    // Retrieve sent file descriptors.
> +    mReceivedFds.AppendElement(ScopedClose(*(static_cast<int*>(CMSG_DATA(chdr)))));
> +

No newline.

@@ +159,4 @@
>  {
> +  // Forget all RAII object to avoid closing the fds.
> +  nsTArray<int> fds;
> +  for (auto &fd : mReceivedFds) {

Put '&' next to 'auto'.

::: ipc/hal/DaemonSocketPDU.h
@@ +9,5 @@
>  
>  #include "mozilla/FileUtils.h"
>  #include "mozilla/ipc/SocketBase.h"
>  #include "mozilla/ipc/DaemonSocketMessageHandlers.h"
>  

No newline here.
Attachment #8722543 - Flags: review?(tzimmermann) → review+
Carry r+ and fixed the nits.
Attachment #8722543 - Attachment is obsolete: true
Attachment #8726030 - Flags: review+
rebase this patch.
Attachment #8726030 - Attachment is obsolete: true
Attachment #8730633 - Flags: review+
Keywords: checkin-needed
How can you check in this patch if the other end (bug 1229298) is not ready yet? Has this ever been tested?
Flags: needinfo?(jacheng)
I think this code is not only used for TV daemon
And I found that the treeherder removed the KK and L emulator, so it has not been tested yet by any test case.

I will remove the |check-in-needed| label and wait for bug 1229298 finished.

I will cowork with Kevin for verifying it.

Thanks
Flags: needinfo?(jacheng)
Keywords: checkin-needed
Cool, thanks! I think Kevin is still looking for bug on the sender side. It's rather non-obvious; I'd guess that all help is welcome.
Hi Thomas,
According to bug 1229298 comment 33 mentioned,
"It only works if all file descriptors are transfered in the same message header",

I modified the patch back to the previous one.

I take your sample code for reference, I think it is reasonable to calculate the FD count by
|size_t fdCount = (chdr->cmsg_len - CMSG_ALIGN(sizeof(struct cmsghdr))) / sizeof(int);|

And as the comment said,

"to pick a reasonably high number that covers all use cases.""

I used a const value and set to 10 for this use case.

Is it OK for you?

This patch is verified by Kevin with the server side TVD and it works.

Thanks
Attachment #8730633 - Attachment is obsolete: true
Attachment #8734352 - Flags: review?(tzimmermann)
Comment on attachment 8734352 [details] [diff] [review]
Make-DaemonSocketPDU-able-to-send-multip.patch

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

Looks good in general, but r- for all the details.

::: dom/bluetooth/bluedroid/BluetoothDaemonSocketInterface.cpp
@@ +221,5 @@
>    {
>      DaemonSocketPDU& pdu = GetPDU();
>  
> +    auto receiveFds = pdu.AcquireFds();
> +    if (receiveFds.Length() == 0) {

This should be contained in NS_WARN_IF.

::: ipc/hal/DaemonSocketPDU.cpp
@@ +20,5 @@
>  #define IODEBUG true
>  #define CHROMIUM_LOG(args...) if (IODEBUG) printf(args);
>  #endif
>  
> +static const int SIZE_OF_POTENTIAL_FD_COUNT = 10;

Please move this constant into |DaemonSocketPDU| and rename it to MAX_NFDS. The type shoudl be 'size_t.' I also suggest to use a multiple of 2, such as 16, for the value.

@@ +119,5 @@
>    memset(&iv, 0, sizeof(iv));
>    iv.iov_base = GetData(0);
>    iv.iov_len = GetAvailableSpace();
>  
> +  uint8_t cmsgbuf[CMSG_SPACE(sizeof(int)) * SIZE_OF_POTENTIAL_FD_COUNT];

Should be declared as

  uint8_t cmsgbuf[CMSG_SPACE(sizeof(int)* MAX_NFDS)];

as all fds are contained in a single header.

@@ +147,5 @@
>      if (NS_WARN_IF(!CMSGHDR_CONTAINS_FD(chdr))) {
>        continue;
>      }
> +    // Retrieve sent file descriptors.
> +    size_t fdCount = (chdr->cmsg_len - CMSG_ALIGN(sizeof(struct cmsghdr))) / sizeof(int);

This computation should probably be contained in a macro; maybe |CMSGHDR_NFDS(chdr)|.

@@ +149,5 @@
>      }
> +    // Retrieve sent file descriptors.
> +    size_t fdCount = (chdr->cmsg_len - CMSG_ALIGN(sizeof(struct cmsghdr))) / sizeof(int);
> +    for (size_t i = 0; i < fdCount; i++) {
> +      mReceivedFds.AppendElement(ScopedClose(*(static_cast<int*>(CMSG_DATA(chdr)) + i)));

That seems hard to read. Rather cast CMSG_DATA(chdr) to an int* and use it here.
Attachment #8734352 - Flags: review?(tzimmermann) → review-
Hi Thomas, 

Thanks for your indicating, I have addressed what you pointed out and verifying this patch by Kevin. 

I cannot find out any MACRO like |CMSGHDR_NFDS| in 
http://lxr.free-electrons.com/source/include/linux/socket.h#L90

so I just leave the computation code.

Please help for the reviewing~

Thanks for your help.
Attachment #8734352 - Attachment is obsolete: true
Attachment #8734641 - Flags: review?(tzimmermann)
Comment on attachment 8734641 [details] [diff] [review]
Make-DaemonSocketPDU-able-to-send-multip.patch

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

r+ with the final nits fixed.

::: dom/bluetooth/bluedroid/BluetoothDaemonSocketInterface.cpp
@@ +282,5 @@
>                                          BluetoothSocketResultHandler* aRes)
>  {
>    /* the file descriptor is attached in the PDU's ancillary data */
> +  auto receiveFds = aPDU.AcquireFds();
> +  int fd = -1;

Please remove this line.

@@ +288,5 @@
> +    ErrorRunnable::Dispatch(aRes, &BluetoothSocketResultHandler::OnError,
> +                            ConstantInitOp1<BluetoothStatus>(STATUS_FAIL));
> +    return;
> +  }
> +  fd = receiveFds[0];

Declare 'int fd' here
Attachment #8734641 - Flags: review?(tzimmermann) → review+
Thanks for reviewing this patch. 

Carry r+ and fix the nit.

Attach the treeherder result for reference,

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b77d19c5c0e2&selectedJob=18771875
Attachment #8734641 - Attachment is obsolete: true
Attachment #8736125 - Flags: review+
Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: