Re: [Qemu-devel] [PATCH v8 7/7] block: Enable qemu_open/close to work with fd sets

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

On 08/10/2012 11:25 AM, Eric Blake wrote:
> On 08/10/2012 08:17 AM, Corey Bryant wrote:
>
>>>> can be closed.  If an fd set has dup() references open, then we
>>>> must keep the other fds in the fd set open in case a reopen
>>>> of the file occurs that requires an fd with a different access
>>>> mode.
>>>>
>>>
>>>
>>> Is this right?  According to the commit message, the whole point of
>>> leaving an fd in the set is to allow the fd to be reused as a dup()
>>> target for as long as the fdset is alive, even if the monitor no longer
>>> cares about the existence of the fd.  But this will always skip over an
>>> fd marked for removal.
>
>>
>> For security purposes, I'm thinking that an fd should no longer be
>> available for opening after libvirt issues a remove-fd command, with no
>> exceptions.
>
> If that's the case, then you don't need the removed flag.  Simply close
> the original fd and forget about it at the point that the monitor

You're right.  Kevin also brought this up to me offline during a 
discussion a little while ago.  I'll plan on closing the fd immediately 
when remove-fd is issued.

> removes the fd.  The fdset will still remain as long as there is a
> refcount, so that libvirt can then re-add to the set.  And that also
> argues that query-fdsets MUST list empty fdsets, where all existing fds
> have been removed, but where the set can still be added to again just
> prior to the next reopen operation.
>
> That is, consider this life cycle:
>
>   libvirt> add-fd opaque="RDWR"
>   qemu< fdset=1, fd=3; the set is in use because of the monitor but with
> refcount 0
>   libvirt> drive_add /dev/fdset/1
>   qemu< success; internally, fd 4 was created as a dup of 3, and fdset 1
> now has refcount 1 and tracks that fd 4 is tied to the set
>   libvirt> remove-fd fdset=1 fd=3, since the drive_add is complete and
> libvirt no longer needs to track what fd qemu is using, but does
> remember internally that qemu is using fdset 1
>   qemu< success; internally, fdset 1 is still refcount 1
>   later on, libvirt prepares to do a snapshot, and knows that after the
> snapshot, qemu will want to reopen the file RDONLY, as part of making it
> the backing image
>   libvirt> add-fd fdset=1 opaque="RDONLY"
>   qemu< fdset=1, fd=3 (yes, fd 3 is available for use again, since the
> earlier remove closed it out)
>   libvirt> blockdev-snapshot-sync
>   qemu< success; internally, the block device knows that /dev/fdset/1 is
> now becoming a backing file, so it tries a reopen to convert its current
> RDWR fd 4 into something RDONLY; the probe succeeds by returning fd 5 as
> a dup of the readonly fd 3
>

This all makes sense to me.

>>
>> Does that work for you?  In that case, the code will remain as-is.
>
> I think that lifecycle will work (either libvirt leaves an fd in the set
> for as long as it thinks qemu might need to do independent open
> operations, or it removes the fd as soon as it knows that qemu is done
> with open, but the fdset remains reserved, tracking all dup'd fds that
> were created from the set.

Yep

>
> For that matter, your refcount doesn't even have to be a separate field,
> but can simply be the length of the list of dup'd fds that are tied to
> the set until a call to qemu_close.  That is, a set must remain alive as
> long as there is either an add-fd that has not yet been removed, or
> there is at least one dup'd fd that has not been qemu_close()d.
>

I agree.  I'll plan on dropping refcount and just checking the dup_fds list.

-- 
Regards,
Corey