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

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.

> 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().

> 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?