From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:51780) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Sm6VU-0001K1-8w for qemu-devel@nongnu.org; Tue, 03 Jul 2012 13:03:25 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Sm6VR-0006ZW-Ke for qemu-devel@nongnu.org; Tue, 03 Jul 2012 13:03:19 -0400 Received: from mx1.redhat.com ([209.132.183.28]:37949) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Sm6VR-0006Ym-9I for qemu-devel@nongnu.org; Tue, 03 Jul 2012 13:03:17 -0400 Message-ID: <4FF325C8.4060401@redhat.com> Date: Tue, 03 Jul 2012 11:03:04 -0600 From: Eric Blake MIME-Version: 1.0 References: <1340390174-7493-1-git-send-email-coreyb@linux.vnet.ibm.com> <20120626091004.GA14451@redhat.com> <4FE9A0F0.2050809@redhat.com> <20120626175045.2c7011b3@doriath.home> <4FEA37A9.10707@linux.vnet.ibm.com> <4FEA3D9C.8080205@redhat.com> <4FF21A67.8010100@linux.vnet.ibm.com> <4FF31265.1000308@linux.vnet.ibm.com> <4FF316C9.5020100@redhat.com> <4FF31CFD.7030508@linux.vnet.ibm.com> In-Reply-To: <4FF31CFD.7030508@linux.vnet.ibm.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="------------enig8446D5A56735947B0F53A08A" Subject: Re: [Qemu-devel] [PATCH v4 0/7] file descriptor passing using pass-fd List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Corey Bryant Cc: Kevin Wolf , aliguori@us.ibm.com, stefanha@linux.vnet.ibm.com, libvir-list@redhat.com, qemu-devel@nongnu.org, Luiz Capitulino , pbonzini@redhat.com This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enig8446D5A56735947B0F53A08A Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable On 07/03/2012 10:25 AM, Corey Bryant wrote: >> I thought qemu would rather return the number of the fdset (which it >> also assigns if none it passed, i.e. for fdset creation). Does libvirt= >> need the number of an individual fd? >> >> If libvirt prefers to assign fdset numbers itself, I'm not against it,= >> it's just something that wasn't clear to me yet. >> >=20 > That's fine. QEMU can return the fdset number or a string > (/dev/fdset/1) if none is specified. And an fdset will need to be > specified if adding to an existing set. >=20 > I think libvirt will need the fd returned by add-fd so that it can > evaluate fds returned by query-fd. It's also useful for remove-fd. Correct - since we will be adding a remove-fd, then that command needs to know both the fdset name and the individual fd within the set to be removed. >=20 >>> 2. drive_add file=3D/dev/fdset/1 -> qemu_open uses the first fd from = the >>> set that has access flags matching the qemu_open action flags. >>> qemu_open increments refcount for this fd. >>> 3. add-fd /dev/fdset/1 FDSET=3D{M} -> qemu adds fd to set named >>> "/dev/fdset/1" - command returns qemu fd to caller (e.g fd=3D5). lib= virt >>> in-use flag turned on for fd. >>> 3. block-commit -> qemu_open reopens "/dev/fdset/1" by using the firs= t >>> fd from the set that has access flags matching the qemu_open action >>> flags. qemu_open increments refcount for this fd. >>> 4. remove-fd /dev/fdset/1 5 -> caller requests fd=3D=3D5 be removed f= rom the >>> set. turns libvirt in-use flag off marking the fd ready to be closed= >>> when qemu is done with it. >> >> If we decided to not return the individual fd numbers to libvirt, file= >> descriptors would be uniquely identified by an fdset/flags pair here. >> >=20 > Are you saying we'd pass the fdset name and flags parameters on > remove-fd to somehow identify the fds to remove? Passing the flag parameters is not trivial, as that would mean the QMP code would have to define constants mapping to all of the O_* flags that qemu_open supports. It's easier to support closing by fd number. >=20 >>> 5. qemu_close decrements refcount for fd, and closes fd when refcount= is >>> zero and libvirt in use flag is off. >> >> The monitor could just hold another reference, then we save the >> additional flag. But that's a qemu implementation detail. >> >=20 > I'm not sure I understand what you mean. pass-fd (or add-fd, whatever name we give it) adds an fd to an fdset, with initial use count of 1 (the use is the monitor). qemu_open() increments the use count. A new qemu_close() wrapper would decrement the use count. And both calling 'remove-fd', or closing the QMP monitor of an fd that has not yet been passed through 'remove-fd', serves as a way to decrement the use count. You'd still have to track whether the monitor is using an fd (to avoid over-decrementing on QMP monitor close), but by having the monitor's use also tracked under the refcount, then refcount reaching 0 is sufficient to auto-close an fd. I think that also means that re-establishing the client QMP connection would increment For some examples: 1. client calls 'add-fd', qemu is now tracking fd=3D4 with refcount 1, in= use by monitor, as member of fdset1 2. client crashes, so all tracked fds are visited; fd=3D4 had not yet bee= n passed to 'remove-fd', so qemu decrements refcount; refcount of fd=3D4 is= now 0 so qemu closes it 1. client calls 'add-fd', qemu is now tracking fd=3D4 with refcount 1, in= use by monitor, as member of fdset1 2. client calls 'device-add' with /dev/fdset/1 as the backing filename, so qemu_open() increments the refcount to 2 3. client crashes, so all tracked fds are visited; fd=3D4 had not yet bee= n passed to 'remove-fd', so qemu decrements refcount to 1, but leaves fd=3D= 4 open because it is still in use by the block device 4. client re-establishes QMP connection, and 'query-fds' lets client learn about fd=3D4 still being open as part of fdset1, but also informs client that fd is not in use by the monitor 1. client calls 'add-fd', qemu is now tracking fd=3D4 with refcount 1, in= use by monitor, as member of fdset1 2. client calls 'device-add' with /dev/fdset/1 as the backing filename, so qemu_open() increments the refcount to 2 3. client calls 'remove-fd fdset=3D1 fd=3D4', so qemu marks fd=3D4 as no longer in use by the monitor, refcount decremented to 1 but still left open because it is in use by the block device 4. client crashes, so all tracked fds are visited; but fd=3D4 is already marked as not in use by the monitor, so its refcount is unchanged 1. client calls 'add-fd', qemu is now tracking fd=3D4 with refcount 1, in= use by monitor, as member of fdset1 2. client calls 'device-add' with /dev/fdset/1 as the backing filename, but the command fails for some other reason, so the refcount is still 1 at the end of the command (although it may have been temporarily incremented then decremented during the command) 3. client calls 'remove-fd fdset=3D1 fd=3D4' to deal with the failure (or= QMP connection is closed), so qemu marks fd=3D4 as no longer in use by th= e monitor, refcount is now decremented to 0 and fd=3D4 is closed I think that covers the idea; you need a bool in_use for tracking monitor state (the monitor is in use until either a remove-fd or a monitor connection closes), as well as a ref-count. >> We also need a query-fdsets command that lists all fdsets that exist. = If >> we add information about single fds to the return value of it, we >> probably don't need a separate query-fd that operates on a single fdse= t. >> >=20 > Yes, good point. And maybe we don't need 2 commands. query-fdsets > could return all the sets and all the fds that are in those sets. Yes, I think a single query command is good enough here, something like: { "execute":"query-fdsets" } =3D> { "return" : { "sets": [ { "name": "fdset1", "fds": [ { "fd": 4, "monitor": true, "refcount": 1 } ] }, { "name": "fdset2", "fds": [ { "fd": 5, "monitor": false, "refcount": 1 }, { "fd": 6, "monitor": true, "refcount": 2 } ] } ] } } >> In use by whom? If it's still in use in qemu (as in "in-use flag would= >> be set") and we have a refcount of zero, then that's a bug. >> >=20 > In use by qemu. I don't think it's a bug. I think there are situation= s > where refcount gets to zero but qemu is still using the fd. I think the refcount being non-zero _is_ what defines an fd as being in use by qemu (including use by the monitor). Any place you have to close an fd before reopening it is dangerous; the safe way is always to open with the new permissions before closing the old permissions. --=20 Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --------------enig8446D5A56735947B0F53A08A Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.12 (GNU/Linux) Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iQEcBAEBCAAGBQJP8yXIAAoJEKeha0olJ0Nqwp0IAKd5eGR/ggb5hIoYLdfjKYZu QWy26cmY1PicmNZY6pRoW90U2i8g80RJC+0EQiUQqfSF3bUcrezHC7eD+REq+QId 1EUArb5Tzq3nwTewdvL3yrj3+zGJhAnPFCsuFrCLRfcMiFu2GGXDPl5SFy0oyS87 QDxGNZ12Q6OsHjCdSVk+7NlL1M/6kJQZ+Hicvk5rvQope+pS3PNJdD7pi8qy/6eE RgwWDkkRiLMrYC+IFrnqqanCoTGflLmYBHjZUG5+eIgxTro74rUwx5h+fDHWFISB maTaidiJb4nF68c5qimnb//XNcqac77tZpaNYl2Co4C4cIepeK7qJ+7ZPBGUP5U= =oPZ6 -----END PGP SIGNATURE----- --------------enig8446D5A56735947B0F53A08A--