On 06/26/2012 04:42 PM, Daniel P. Berrange wrote:
On Tue, Jun 26, 2012 at 02:40:03PM -0400, Corey Bryant wrote:
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.
Hmm, I'm not sure I understand what the use case is for the 'force'
parameter ? In my proof of concept I left out the block of code
from qemu_open() you had for dup'ing the FD with various different
flags set, but that was just for brevity. I think it ought to fit
in the same way, unless you're refering to a different area of the
code.
The access modes (O_RDONLY, O_WRONLY, or O_RDWR) can only be set on the
open() call, which is performed by libvirt. fcntl(F_SETFL) can't change
them. So the point of pass-fd's force option is to allow libvirt to
open() the same file with a new access mode, pass it to qemu, and copy
it to the existing qemu fd. For example:
fd1 = open("/path/to/file.img", O_RDONLY)
fd2 = open("/path/to/file.img", O_RDWR)
pass-fd disk0 (fd1) returns fd=3 (/dev/fd/3 in qemu has O_RDONLY)
drive_add file:/dev/fd/3
pass-fd disk0 (fd2) returns fd=3 (/dev/fd/3 in qemu now has O_RDWR)
This is required because the access_mode flags used by qemu_open() are
the same as the passed fd's access_mode flags. And qemu will re-open
files with different access modes.
@@ -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 */
eg, this part of the code you had some work related to
'flags'
Yes, no problem and thanks again for the code. :) I'm hoping we can get
some more input soon so I can get moving in one direction or another.
+ return fd;
+ }
if (flags & O_CREAT) {
va_list ap;
Daniel
--
Regards,
Corey
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list