Re: [libvirt] [Qemu-devel] [PATCH v4 0/7] file descriptor passing using pass-fd

On 06/26/2012 11:37 AM, Corey Bryant wrote: > > >On 06/26/2012 11:03 AM, Daniel P. Berrange wrote: >>On Tue, Jun 26, 2012 at 03:11:40PM +0100, Daniel P. Berrange wrote: >>>On Tue, Jun 26, 2012 at 09:52:51AM -0400, Corey Bryant wrote: >>>>>So now from a client's POV you'd have a flow like >>>>> >>>>> * drive_add "file=/dev/fd/N" FDSET={N} >>>> >>>>IIUC then drive_add would loop and pass each fd in the set via >>>>SCM_RIGHTS? >>> >>>Yes, you'd probably use the JSON to tell QEMU exactly >>>how many FDs you're intending to pass with the command, >>>and then once the command is received, you'd have N*SCM_RIGHTS >>>messages sent/received. >>> >>>>> >>>>>And in QEMU you'd have something like >>>>> >>>>> * handle_monitor_command >>>>> - recvmsg all FDs, and stash them in a thread local >>>>>"FDContext" >>>>> context >>>>> - invoke monitor command handler >>>>> - Sees file=/dev/fd/N >>>>> - Fetch /dev/fd/N from "FDContext" >>>>> - If success remove /dev/fd/N from "FDContext" >>>>> - close() all FDs left in "FDContext" >>>>> >>>>>The key point with this is that because the FDs are directly >>>>>associated with a monitor command, QEMU can /guarantee/ that >>>>>FDs are never leaked, regardless of client behaviour. >>>> >>>>Wouldn't this leak fds if libvirt crashed part way through sending >>>>the set of fds? >>> >>>No, because you've scoped the FDs to the current monitor instance, >>>and the current command being processed you know to close all FDs >>>when the associated command has finished, as well as closing them >>>all if you see EOF on the monitor socket while in the middle of >>>receiving a command. >> >>Here is a quick proof of concept (ie untested) patch to demonstrate >>what I mean. It relies on Cory's patch which converts everything >>to use qemu_open. It is also still valuable to make the change >>to qemu_open() to support "/dev/fd/N" for passing FDs during >>QEMU initial startup for CLI args. >> >>IMHO, what I propose here is preferrable for QMP clients that >>our current plan of requiring use of 3 monitor commands (passfd, >>XXXXX, closefd). > >Thanks for the PoC. > >Two other required updates that I can think of would be: > >1) Update change, block_stream, block_reopen, snapshot_blkdev, and >perhaps other monitor commands to support receiving fd's via SCM_RIGHTS. > Nevermind my comment. I see that your PoC supports passing nfds for any QMP command. I'm curious what Kevin's thoughts are on this and the overall approach. >2) Support re-opening files with different access modes (O_RDONLY, >O_WRONLY, or O_RDWR). This would be similar to the force option for >pass-fd. > I'm still not quite sure how we'd go about this. We need a way to determine the existing QEMU fd that is to be re-associated with the new fd, when using a /dev/fdlist/0 filename. In this new approach, 0 corresponds to an index, not an fd. The prior approach of using the /dev/fd/nnn, where nnn corresponded to an actual QEMU fd, made this easy.

> >>@@ -83,6 +158,26 @@ int qemu_open(const char *name, int flags, ...) >> { >> int ret; >> int mode = 0; >>+ const char *p; >>+ >>+ /* Attempt dup of fd for pre-opened file */ >>+ if (strstart(name, "/dev/fdlist/", &p)) { >>+ int idx; >>+ int fd; >>+ >>+ idx = qemu_parse_fd(p); >>+ if (idx == -1) { >>+ return -1; >>+ } >>+ >>+ fd = qemu_fdlist_dup(idx); >>+ if (fd == -1) { >>+ return -1; >>+ } >>+ >>+ /* XXX verify flags match */

>>+ return fd; >>+ } >> >> if (flags & O_CREAT) { >> va_list ap;