From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:53019) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SzrOf-0000jx-Is for qemu-devel@nongnu.org; Fri, 10 Aug 2012 11:45:11 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SzrOb-00089L-TB for qemu-devel@nongnu.org; Fri, 10 Aug 2012 11:45:09 -0400 Received: from e9.ny.us.ibm.com ([32.97.182.139]:51557) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SzrOb-00088Y-Ox for qemu-devel@nongnu.org; Fri, 10 Aug 2012 11:45:05 -0400 Received: from /spool/local by e9.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 10 Aug 2012 11:45:04 -0400 Received: from d01relay03.pok.ibm.com (d01relay03.pok.ibm.com [9.56.227.235]) by d01dlp03.pok.ibm.com (Postfix) with ESMTP id BEE31C9005E for ; Fri, 10 Aug 2012 11:45:00 -0400 (EDT) Received: from d01av02.pok.ibm.com (d01av02.pok.ibm.com [9.56.224.216]) by d01relay03.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q7AFj0xY108002 for ; Fri, 10 Aug 2012 11:45:00 -0400 Received: from d01av02.pok.ibm.com (loopback [127.0.0.1]) by d01av02.pok.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q7AFiugv024202 for ; Fri, 10 Aug 2012 12:44:57 -0300 Message-ID: <50252C78.4000801@linux.vnet.ibm.com> Date: Fri, 10 Aug 2012 11:44:56 -0400 From: Corey Bryant MIME-Version: 1.0 References: <1344564649-6272-1-git-send-email-coreyb@linux.vnet.ibm.com> <1344564649-6272-8-git-send-email-coreyb@linux.vnet.ibm.com> <5024A725.50107@redhat.com> <50251801.3060204@linux.vnet.ibm.com> <502527D1.1040404@redhat.com> In-Reply-To: <502527D1.1040404@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v8 7/7] block: Enable qemu_open/close to work with fd sets List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: kwolf@redhat.com, aliguori@us.ibm.com, stefanha@linux.vnet.ibm.com, libvir-list@redhat.com, qemu-devel@nongnu.org, lcapitulino@redhat.com, pbonzini@redhat.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