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

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.