[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 19:19:08 UTC 2012
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
More information about the libvir-list
mailing list