Re: [Qemu-devel] [PATCH v4 0/7] file descriptor passing using pass-fd

[Kevin Wolf <kwolf@redhat.com>]

Am 03.07.2012 00:31, schrieb Eric Blake:
> On 07/02/2012 04:02 PM, Corey Bryant wrote:
> 
>> Here's another option that Kevin and I discussed today on IRC.  I've
>> modified a few minor details since the discussion. And Kevin please
>> correct me if anything is wrong.
>>
>> Proposal Four: Pass a set of fds via 'pass-fds'.  The group of fds
>> should all refer to the same file, but may have different access flags
>> (ie. O_RDWR, O_RDONLY).  qemu_open can then dup the fd that has the
>> matching access mode flags.
> 
> But this means that libvirt has to open a file O_RDWR up front for any
> file that it _might_ need qemu to reopen later, and that qemu is now
> hanging on to 2 fds per fdset instead of 1 fd for the life of any client
> of the fdset.

It doesn't have to be up front. There's no reason not to have monitor
commands that add or remove fds from a given fdset later.

> I see no reason why libvirt can't pass in an O_RDWR fd when qemu only
> needs to use an O_RDONLY fd; making qemu insist on an O_RDONLY fd makes
> life harder for libvirt.  On the other hand, I can see from a safety
> standpoint that passing in an O_RDWR fd opens up more potential for
> errors than passing in an O_RDONLY fd, 

Yes, this is exactly my consideration. Having a read-only file opened as
O_RDWR gives us the chance to make stupid mistakes.

> but if you don't know up front
> whether you will ever need to write into a file, then it would be better
> to pass in O_RDONLY.  At least I don't see it as a security risk:
> passing in O_RDWR but setting SELinux permissions on the fd to only
> allow read() but not write() via the labeling of the fd may be possible,
> so that libvirt could still prevent accidental writes into an O_RDWR
> file that starts life only needing to service reads.

But this would assume that libvirt knows exactly when a reopen happens,
for each current and future qemu version. I wouldn't want to tie qemu's
internals so closely to the management application, even if libvirt
could probably get it reasonably right (allow rw before sending a
monitor command; revoke rw when receiving a QMP event that the commit
has completed). It wouldn't work if we had a qemu-initiated ro->rw
transition, but I don't think we have one at the moment.

>> pass-fds:
>>     { 'command': 'pass-fds',
>>       'data': {'fdsetname': 'str', '*close': 'bool'},
>>       'returns': 'string' }
> 
> This still doesn't solve Dan's complaint that things are not atomic; if
> the monitor dies between the pass-fds and the later use of the fdset, we
> would need a counterpart monitor command to query what fdsets are
> currently known by qemu. 

If you want a query-fdsets, that should be easy enough.

Actually, we might be able to even make the command transactionable.
This would of course require blockdev-add to be transactionable as well
to be of any use.

> And like you pointed out, you didn't make it
> clear how a timeout mechanism would be implemented to auto-close fds
> that are not consumed in a fixed amount of time - would that be another
> optional parameter to 'pass-fds'?

Do you think a timeout is helpful? Can't we just drop libvirt's
reference automatically if the monitor connection gets closed?

The BH/timer thing Corey mentioned is more about the qemu internal
problem that during a reopen there may be a short period where the old
fd is closed, but the new one not yet opened, so the fdset might need to
survive a short period with refcount 0.

> Or do we need a way to initially create a set with only one O_RDONLY fd,
> but later pass in a new O_RDWR fd but associate it with the existing set
> rather than creating a new set (akin to the 'force' option of 'pass-fd'
> in proposal two)?

As I said above, yes, I think this makes a lot sense.

Kevin