From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35031) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zljj9-00083p-Qr for qemu-devel@nongnu.org; Mon, 12 Oct 2015 16:29:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zljj8-0002co-As for qemu-devel@nongnu.org; Mon, 12 Oct 2015 16:29:47 -0400 References: <5cb22bcd1ae6ca976ae6d2b823316c557ad0c08e.1444640617.git.berto@igalia.com> From: Max Reitz Message-ID: <561C182F.7060203@redhat.com> Date: Mon, 12 Oct 2015 22:29:35 +0200 MIME-Version: 1.0 In-Reply-To: <5cb22bcd1ae6ca976ae6d2b823316c557ad0c08e.1444640617.git.berto@igalia.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="d3UsR6tMBjRTxCwgD53kv1lJCEHauFqCn" Subject: Re: [Qemu-devel] [PATCH v7 4/5] block: add a 'blockdev-snapshot' QMP command List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alberto Garcia , qemu-devel@nongnu.org Cc: Kevin Wolf , Stefan Hajnoczi , qemu-block@nongnu.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --d3UsR6tMBjRTxCwgD53kv1lJCEHauFqCn Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: quoted-printable On 12.10.2015 11:16, Alberto Garcia wrote: > One of the limitations of the 'blockdev-snapshot-sync' command is that > it does not allow passing BlockdevOptions to the newly created > snapshots, so they are always opened using the default values. >=20 > Extending the command to allow passing options is not a practical > solution because there is overlap between those options and some of > the existing parameters of the command. >=20 > This patch introduces a new 'blockdev-snapshot' command with a simpler > interface: it just takes two references to existing block devices that > will be used as the source and target for the snapshot. >=20 > Since the main difference between the two commands is that one of them > creates and opens the target image, while the other uses an already > opened one, the bulk of the implementation is shared. >=20 > Signed-off-by: Alberto Garcia > Cc: Eric Blake > Reviewed-by: Max Reitz > --- > blockdev.c | 165 ++++++++++++++++++++++++++++++++-----------= -------- > qapi-schema.json | 2 + > qapi/block-core.json | 28 +++++++++ > qmp-commands.hx | 38 ++++++++++++ > 4 files changed, 172 insertions(+), 61 deletions(-) >=20 > diff --git a/blockdev.c b/blockdev.c > index 12741a0..b5470c9 100644 > --- a/blockdev.c > +++ b/blockdev.c [...] > @@ -1521,58 +1533,48 @@ typedef struct ExternalSnapshotState { [...] > } > =20 > /* start processing */ > - state->old_bs =3D bdrv_lookup_bs(has_device ? device : NULL, > - has_node_name ? node_name : NULL, > - &local_err); > - if (local_err) { > - error_propagate(errp, local_err); > - return; > - } > - > - if (has_node_name && !has_snapshot_node_name) { > - error_setg(errp, "New snapshot node name missing"); > - return; > - } > - > - if (has_snapshot_node_name && > - bdrv_lookup_bs(snapshot_node_name, snapshot_node_name, NULL)) = { > - error_setg(errp, "New snapshot node name already in use"); There's a difference from v6 here... > + state->old_bs =3D bdrv_lookup_bs(device, node_name, errp); > + if (!state->old_bs) { > return; > } > =20 > @@ -1602,35 +1604,70 @@ static void external_snapshot_prepare(BlkTransa= ctionState *common, > return; > } > =20 > - flags =3D state->old_bs->open_flags; > + if (action->kind =3D=3D TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_= SYNC) { > + BlockdevSnapshotSync *s =3D action->blockdev_snapshot_sync; > + const char *format =3D s->has_format ? s->format : "qcow2"; > + enum NewImageMode mode; > + const char *snapshot_node_name =3D > + s->has_snapshot_node_name ? s->snapshot_node_name : NULL; > =20 > - /* create new image w/backing file */ > - if (mode !=3D NEW_IMAGE_MODE_EXISTING) { > - bdrv_img_create(new_image_file, format, > - state->old_bs->filename, > - state->old_bs->drv->format_name, > - NULL, -1, flags, &local_err, false); > - if (local_err) { > - error_propagate(errp, local_err); > + if (node_name && !snapshot_node_name) { > + error_setg(errp, "New snapshot node name missing"); > return; > } > - } > =20 > - options =3D qdict_new(); > - if (has_snapshot_node_name) { > - qdict_put(options, "node-name", > - qstring_from_str(snapshot_node_name)); > + if (snapshot_node_name && > + bdrv_lookup_bs(snapshot_node_name, snapshot_node_name, NUL= L)) { > + error_setg(errp, "New snapshot node name already in use");= =2E..and here, but how to resolve the conflict resulting from the newly added patch 1 was obvious, so my R-b stands, of course. Anyway, this is not why I'm replying, that's further down: > + return; > + } > + > + flags =3D state->old_bs->open_flags; > + > + /* create new image w/backing file */ > + mode =3D s->has_mode ? s->mode : NEW_IMAGE_MODE_ABSOLUTE_PATHS= ; > + if (mode !=3D NEW_IMAGE_MODE_EXISTING) { > + bdrv_img_create(new_image_file, format, > + state->old_bs->filename, > + state->old_bs->drv->format_name, > + NULL, -1, flags, &local_err, false); > + if (local_err) { > + error_propagate(errp, local_err); > + return; > + } > + } > + > + options =3D qdict_new(); > + if (s->has_snapshot_node_name) { > + qdict_put(options, "node-name", > + qstring_from_str(snapshot_node_name)); > + } > + qdict_put(options, "driver", qstring_from_str(format)); > + > + flags |=3D BDRV_O_NO_BACKING; > } > - qdict_put(options, "driver", qstring_from_str(format)); > =20 > - /* TODO Inherit bs->options or only take explicit options with an > - * extended QMP command? */ > assert(state->new_bs =3D=3D NULL); > - ret =3D bdrv_open(&state->new_bs, new_image_file, NULL, options, > - flags | BDRV_O_NO_BACKING, &local_err); > + ret =3D bdrv_open(&state->new_bs, new_image_file, snapshot_ref, op= tions, > + flags, errp); > /* We will manually add the backing_hd field to the bs later */ > if (ret !=3D 0) { > - error_propagate(errp, local_err); > + return; > + } > + > + if (state->new_bs->blk !=3D NULL) { > + error_setg(errp, "The snapshot is already in use by %s", > + blk_name(state->new_bs->blk)); > + return; > + } > + > + if (bdrv_op_is_blocked(state->new_bs, BLOCK_OP_TYPE_EXTERNAL_SNAPS= HOT, > + errp)) { > + return; > + } > + > + if (state->new_bs->backing_hd !=3D NULL) { > + error_setg(errp, "The snapshot already has a backing image"); > } It's here: In case Kevin's series is applied before this one (which I'm assuming since you were brave enough to base this series on my BB series), this needs to be s/backing_hd/backing/. I'm just saying this so you know you can keep my R-b then. Max > } > =20 --d3UsR6tMBjRTxCwgD53kv1lJCEHauFqCn Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBCAAGBQJWHBgvAAoJEDuxQgLoOKytKcQH/iMGEb9nWdIzh8+xDFbIZzgY WNyD1xkiec28edFi6h7xX4B0A1uWIy6dBz+Bap4f6LlhY21H59gCp/Oq3dXH1XWJ iisP/Krt3VJPBwyAXE+3TEjAMJI268a3Xt/PPYXJ6H1el3cMGKuSb8yV/Us6ozeb Fi5r/a2svKkeDW/QuvLA6RqeDqoQPdziYY0/+IxonxM4CLHvgOrMWNcLhqVus/Fz vkauPnUsN0+UUoC5xY0SuxdM1BaUQzNiGEgJ3XNkIsBvK70VX9DcB9h9ozj/3gMz BgXwFj6JupgM0iy/VLpEuq83Nmu/gXUCVEmUwASbaozk2P+zORJZ74nqrrQqULc= =YCRM -----END PGP SIGNATURE----- --d3UsR6tMBjRTxCwgD53kv1lJCEHauFqCn--