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

[Corey Bryant <coreyb@linux.vnet.ibm.com>]

On 06/15/2012 04:00 PM, Eric Blake wrote:
> On 06/15/2012 01:19 PM, Corey Bryant wrote:
>
>>>> 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.
>
> In those situations, libvirt would pass fd with O_RDWR, and qemu_open()
> would be fine requesting O_RDONLY the first time (subset is okay), and
> O_RDWR the second time.  Where you have to error out is where libvirt
> passes O_RDONLY but qemu wants O_RDWR, and so forth.
>

I'll plan on going with this approach.

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

s/dopes/does

>> it's ok to set FD_CLOEXEC for getfd too, then I'm happy to do it.
>
> The only case that a client might break is if there were a way to pass
> an fd into qemu and then intentionally see that fd in a child process of
> qemu.  But in the case of 'migrate fd:nnn', you aren't spawning a child
> process, and even in the case of 'migrate exec:command' (which libvirt
> no longer uses if fd:nnn works), I don't see how the client could have
> ever intentionally tried to use 'getfd' in advance to pass an extra fd
> for use inside the 'exec:command' child.  Besides, before 'pass-fd' was
> around, how would the management app triggering the 'exec:command' even
> know what fd number would accidentally be inherited into the
> exec:command child?  I think it is pretty much a straight bug-fix for
> 'getfd' to always set FD_CLOEXEC, and preferably set it atomically via
> MSG_CMSG_CLOEXEC.
>

Alright, I'll go ahead and make this update in the next version of the 
patch series.

Thanks for all the input!

-- 
Regards,
Corey