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

Re: [libvirt] [PATCH v3 3/5] osdep: Enable qemu_open to dup pre-opened fd





On 06/15/2012 02:46 PM, Kevin Wolf wrote:
Am 15.06.2012 20:16, schrieb Corey Bryant:


On 06/15/2012 11:16 AM, Eric Blake wrote:
On 06/14/2012 09:55 AM, Corey Bryant wrote:
This patch adds support to qemu_open to dup(fd) a pre-opened file
descriptor if the filename is of the format /dev/fd/X.


+++ b/osdep.c
@@ -82,6 +82,19 @@ int qemu_open(const char *name, int flags, ...)
       int ret;
       int mode = 0;

+#ifndef _WIN32
+    const char *p;
+
+    /* Attempt dup of fd for pre-opened file */
+    if (strstart(name, "/dev/fd/", &p)) {
+        ret = qemu_parse_fd(p);
+        if (ret == -1) {
+            return -1;
+        }
+        return dup(ret);

I think you need to honor flags so that the end use of the fd will be as
if qemu had directly opened the file, rather than just doing a blind dup
with a resulting fd that is in a different state than the caller
expected.  I can think of at least the following cases (there may be more):

I was thinking libvirt would handle all the flag settings on open
(obviously since that's how I coded it).  I think you're right with this
approach though as QEMU will re-open the same file various times with
different flags.

There are some flags that I don't think we'll be able to change.  For
example: O_RDONLY, O_WRONLY, O_RDWR.  I assume libvirt would open all
files O_RDWR.

I think we need to check all of them and fail qemu_open() if they don't
match. Those that qemu can change, should be just changed, of course.


Ok. I remember a scenario where QEMU opens a file read-only (perhaps to check headers and determine the file format) before re-opening it read-write. Perhaps this is only when format= isn't specified with -drive. I'm thinking we may need to change flags to read-write where they used to be read-only, in some circumstances.

Oh, and are we using MSG_CMSG_CLOEXEC where possible (and where not
possible, falling back to fcntl(F_GETFD/F_SETFD) to set FD_CLOEXEC) on
all fds received by 'getfd' and 'pass-fd'?  I can't think of any reason
why 'migrate fd:name' would need to be inheritable, and in the case of
/dev/fd/ parsing, while the dup() result may need to be inheritable, the
original that we are dup'ing from should most certainly be cloexec.

It doesn't look like we use MSG_CMSG_CLOEXEC anywhere in QEMU.  I don't
think we can modify getfd at this point (compatibility) but we could
update pass-fd to set it.  It may make more sense to set it with
fcntl(FD_CLOEXEC) in qmp_pass_fd().

In which scenario would any client break if we set FD_CLOEXEC? I don't
think compatibility means we can't fix any bugs.


I don't know if it breaks any client. Maybe it's not a compatibility error. It dopes change behavior down the line though. If you think it's ok to set FD_CLOEXEC for getfd too, then I'm happy to do it.

if (flags & O_NONBLOCK)
     use fcntl(F_GETFL/F_SETFL) to set O_NONBLOCK
else
     use fcntl(F_GETFL/F_SETFL) to clear O_NONBLOCK

or maybe we document that callers of pass-fd must always pass fds with
O_NONBLOCK clear instead of clearing it ourselves.  Or maybe we make
sure part of the process of tying name with fd in the lookup list of
named fds is determining the current O_NONBLOCK state in case future
qemu_open() need it in the opposite state.

Just documenting it seems error-prone.  Why not just set/clear it based
on the flag passed to qemu_open?

I agree. We could just check and return an error if they aren't set
correctly, but I think adjusting the flags is nicer.

Kevin


Ok thanks for the input!

--
Regards,
Corey



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