[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 5/8] Extend RPC protocol to allow FD passing

On 10/25/2011 10:03 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange"<berrange redhat com>

Define two new RPC message types VIR_NET_CALL_WITH_FDS and
VIR_NET_REPLY_WITH_FDS. These message types are equivalent
to VIR_NET_CALL and VIR_NET_REPLY, except that between the
message header, and payload there is a 32-bit integer field
specifying how many file descriptors have been passed.

The actual file descriptors are sent/recv'd out of band.

* src/rpc/virnetmessage.c, src/rpc/virnetmessage.h,
   src/libvirt_private.syms: Add support for handling
   passed file descriptors
* src/rpc/virnetprotocol.x: Extend protocol for FD
  docs/internals/rpc.html.in |   33 +++++++++++++

Thanks; this fixes my biggest concern from the last round.

+      With the two packet types that support passing file descriptors, in
+      between the header and the payload there will be a 4-byte integer
+      specifying the number of file descriptors which are being sent.
+      The actual file handles are sent after the payload has been sent.
+      Each file handle has a single dummy byte transmitted as a carrier
+      for the out of band file descriptor.

gnulib recvfd() ignores this byte value. While sendfd() sends NUL, it is conceivable that a 3rd-party implementation of the wire protocol might pick a different value (or that gnulib might change in the future). Should we add any documentation along the lines of 'sender should send 0' and 'receiver must ignore the dummy byte, even if it is not 0', just for robustness sake?

+int virNetMessageDupFD(virNetMessagePtr msg,
+                       size_t slot)
+    int fd;
+    if (slot>= msg->nfds) {
+        virNetError(VIR_ERR_INTERNAL_ERROR,
+                    _("No FD available at slot %zu"), slot);
+        return -1;
+    }
+    if ((fd = dup(msg->fds[slot]))<  0) {
+        virReportSystemError(errno,
+                             _("Unable to duplicate FD %d"),
+                             msg->fds[slot]);
+        return -1;
+    }

Do we want to be using gnulib's dup_cloexec ("cloexec.h") or fcntl(fd, F_DUPFD_CLOEXEC, 0) here, so that our dup doesn't leak into a third-party child if we are linked into some larger multithreaded app?

ACK with that resolved (and I'm okay if the resolution is saving the conversion of dup to dup_cloexec for another day, especially since I didn't raise the issue on the last round of reviews, but want this to go in before code freeze).

Eric Blake   eblake redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]