[libvirt] [PATCH v3 3/5] osdep: Enable qemu_open to dup pre-opened fd
Corey Bryant
coreyb at linux.vnet.ibm.com
Fri Jun 15 18:16:59 UTC 2012
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.
>
> if (flags & O_TRUNCATE ||
> ((flags & (O_CREAT|O_EXCL)) == (O_CREAT|O_EXCL))
> ftruncate(ret, 0);
>
> Why do I truncate on O_CREAT|O_EXCL? Because that's a case where open()
> guarantees that the file will have just been created empty.
>
That makes sense.
> if (flags & O_CLOEXEC)
> use fcntl(F_DUPFD_CLOEXEC) instead of dup(), if possible
> else fallback to fcntl(F_GETFD/F_SETFD) to set FD_CLOEXEC non-atomically
>
That makes sense too.
> 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?
>
> Treat O_APPEND similarly to O_NONBLOCK (with fcntl(F_GETFL/F_SETFL) to
> match the desired open() setting).
>
Ok
--
Regards,
Corey
More information about the libvir-list
mailing list