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

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

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.

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org