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

[Kevin Wolf <kwolf@redhat.com>]

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.

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

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