From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51820) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WkxnV-0002eq-E3 for qemu-devel@nongnu.org; Thu, 15 May 2014 11:42:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WkxnQ-0002YQ-8R for qemu-devel@nongnu.org; Thu, 15 May 2014 11:42:17 -0400 Received: from mx1.redhat.com ([209.132.183.28]:63865) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WkxnP-0002YL-U5 for qemu-devel@nongnu.org; Thu, 15 May 2014 11:42:12 -0400 Message-ID: <5374E04F.6010205@redhat.com> Date: Thu, 15 May 2014 09:42:07 -0600 From: Eric Blake MIME-Version: 1.0 References: <13ae3f12913b517c7f2e5e356b54b6bf872045da.1400123059.git.jcody@redhat.com> In-Reply-To: <13ae3f12913b517c7f2e5e356b54b6bf872045da.1400123059.git.jcody@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="bxg1CHUecAwwv28p1uK3Cu5HrwMptup3h" Subject: Re: [Qemu-devel] [PATCH 4/5] block: Accept node-name arguments for block-commit List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jeff Cody , qemu-devel@nongnu.org Cc: kwolf@redhat.com, benoit.canet@irqsave.net, pkrempa@redhat.com, famz@redhat.com, stefanha@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --bxg1CHUecAwwv28p1uK3Cu5HrwMptup3h Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 05/14/2014 09:20 PM, Jeff Cody wrote: > This modifies the block operation block-commit so that it will > accept node-name arguments for either 'top' or 'base' BDS. >=20 > The filename and node-name are mutually exclusive to each other; > i.e.: > "top" and "top-node-name" are mutually exclusive (enforced) > "base" and "base-node-name" are mutually exclusive (enforced) Hmm - we have a bug in existing commands for NOT forcing mutual exclusion even though the schema says they are exclusive. For example, qmp_block_passwd() blindly calls: bs =3D bdrv_lookup_bs(has_device ? device : NULL, has_node_name ? node_name : NULL, &local_err); and _that_ function blindly favors device name over node name, rather than erroring out if both are supplied. I think we should fix that first - I'd rather that bdrv_lookup_bs either errors out if both names are supplied (rather than making each caller repeat the work), or that it checks that if both names are supplied then it resolves to the same bs= =2E Hmm - if I have device "foo" with chain "base <- top", would bdrv_lookup_bs("foo", "base") be okay ("base" is a node-name that resides within "device"'s chain) or an error ("base" resolves to a different bs than "device")? Again, all the more reason to have a common function decide the semantics we want, then all other clients automatically pick up on those semantics. >=20 > The preferred and recommended method now of using block-commit > is to specify the BDS to operate on via their node-name arguments. >=20 > This also adds an explicit check that base_bs is in the chain of > top_bs, because if they are referenced by their unique node-name > arguments, the discovery process does not intrinsically verify > they are in the same chain. I like this addition. >=20 > Signed-off-by: Jeff Cody > --- > blockdev.c | 35 +++++++++++++++++++++++++++++++++-- > qapi-schema.json | 35 ++++++++++++++++++++++++++--------- > qmp-commands.hx | 29 ++++++++++++++++++++++------- > 3 files changed, 81 insertions(+), 18 deletions(-) >=20 > =20 > - if (top) { > + /* Find the 'top' image file for the commit */ > + if (has_top_node_name) { > + top_bs =3D bdrv_lookup_bs(NULL, top_node_name, &local_err); Hmm. Shouldn't you ALSO verify that 'top_bs' belongs to the chain owned by 'device'? Later on, you verify that 'base_bs' belongs to 'top_bs', but if I pass node names, those names could have found a top unrelated to 'device'; and base related to that top. Maybe that gives more weight to my idea of possibly allowing bdrv_lookup_bs(device, top_node_name), where the lookup succeeds even when device and top_node_name resolve to different bs, but where top_node_name is a node in the chain of device. >=20 > - if (has_base && base) { > + /* Find the 'base' image file for the commit */ > + if (has_base_node_name) { > + base_bs =3D bdrv_lookup_bs(NULL, base_node_name, &local_err); > + if (local_err) { > + error_propagate(errp, local_err); > + return; > + } > + } else if (has_base && base) { Here, it doesn't matter whether you pass device or NULL for the device name... > =20 > + /* Verify that 'base' is in the same chain as 'top' */ > + if (!bdrv_is_in_chain(top_bs, base_bs)) { > + error_setg(errp, "'base' and 'top' are not in the same chain")= ; > + return; =2E..because this check is still mandatory to prove that a user with chai= n: base <- sn1 <- sn2 <- active is not calling 'device':'active', 'top-node-name':'sn1', 'base-node-name':'sn2' (all that bdrv_lookup_bs can do is verify that base-node-name belongs to device, not that it _also_ belongs to top_bs). > -# @base: #optional The file name of the backing image to write data = into. > -# If not specified, this is the deepest backing ima= ge > +# For 'base', either @base or @base-node-name may be set but not both.= If > +# neither is specified, this is the deepest backing image I like how you factored th is out up front... > # > -# @top: #optional The file name of the backing image within the ima= ge chain, > -# which contains the topmost data to be committed d= own. If > -# not specified, this is the active layer. > +# @base: #optional The file name of the backing image to writ= e data > +# into. > +# > +# @base-node-name #optional The name of the block driver state node of= the > +# backing image to write data into. > +# (Since 2.1) > +# > +# For 'top', either @top or @top-node-name must be set but not both. =2E..but here, you were not consistent. I'd add "If neither is specified= , this is the active image" here... > +# > +# @top: #optional The file name of the backing image within = the image > +# chain, which contains the topmost data to = be > +# committed down. If not specified, this is = the > +# active layer. =2E..and drop the second sentence from here. > +# > +# @top-node-name: #optional The block driver state node name of the ba= cking > +# image within the image chain, which contai= ns the > +# topmost data to be committed down. > +# (Since 2.1) > # > # If top =3D=3D base, that is an error. > # If top =3D=3D active, the job will not be complet= ed by itself, > @@ -2120,17 +2135,19 @@ > # > # Returns: Nothing on success > # If commit or stream is already active on this device, Devic= eInUse > -# If @device does not exist, DeviceNotFound > +# If @device does not exist or cannot be determined, DeviceNo= tFound > # If image commit is not supported by this device, NotSupport= ed > -# If @base or @top is invalid, a generic error is returned > +# If @base, @top, @base-node-name, @top-node-name invalid, Ge= nericError > # If @speed is invalid, InvalidParameter > +# If both @base and @base-node-name are specified, GenericErr= or > +# If both @top and @top-node-name are specified, GenericError= These last two are arguably sub-conditions of the earlier statement about @top and @top-node-name being invalid (since being invalid can include when both strings are used at once). It wouldn't hurt my feelings to reduce the docs by two lines. > # Since: 1.3 > # > ## > { 'command': 'block-commit', > - 'data': { 'device': 'str', '*base': 'str', '*top': 'str', > - '*speed': 'int' } } > + 'data': { 'device': 'str', '*base': 'str', '*base-node-name': 'str',= > + '*top': 'str', '*top-node-name': 'str', '*speed': 'int' } = } I'm okay with keeping 'device' mandatory, even though technically the use of a node-name could imply which device is intended. That is, as long as the code correctly errors out when device and top-node-name don't resolve to the same chain :) > + > +For 'top', either top or top-node-name must be set but not both. > + > +- "top": The file name of the backing image within the imag= e chain, > + which contains the topmost data to be committed do= wn. If > + not specified, this is the active layer. > + (json-string, optional) Same blurb about hoisting the 'not specified =3D> active' sentence to the= common text, rather than leaving it in the 'top' text. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --bxg1CHUecAwwv28p1uK3Cu5HrwMptup3h 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 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJTdOBPAAoJEKeha0olJ0Nq1XcH/RZfSWhhcxb5zk/LHX0RQK72 PeECsjbJ49HHA+INDi4k+BMa/gtAS2CPRhHwSHZOAYyOztO4a16yQGCRULzt5hSj jWPicgeVXk5JLLjNUcBvMwCwynojk8mnNfj38PssO4y7O2Zz4nYoG4jlYFjFEePK /PZQ6/BZkoCdvxFQWiqnucACFUVjI+x1wYIPmXKM5hlqZDvFSR91rYBzzYiJLGpz Two5zY+Dp2sfDA/TgGgeAvbCwBp09pthj7uNwLk59dHN+nKfPIopbCHin7+aSzeu uKPqhZRb7AK3+DHZBZhZ9vl+Ei9qS85m6bv6GW4+fKyF5caSEgbv5QpSoy5bSBc= =dNN5 -----END PGP SIGNATURE----- --bxg1CHUecAwwv28p1uK3Cu5HrwMptup3h--