Re: [Qemu-devel] [PATCH v6 3/4] block: add a 'blockdev-snapshot' QMP command

[Kevin Wolf]

Am 06.10.2015 um 17:49 hat Alberto Garcia geschrieben:
> On Tue 06 Oct 2015 05:30:07 PM CEST, Kevin Wolf wrote:
> >> -options = qdict_new();
> >> -if (has_snapshot_node_name) {
> >> -qdict_put(options, "node-name",
> >> -  qstring_from_str(snapshot_node_name));
> >> +if (snapshot_node_name && bdrv_find_node(snapshot_node_name)) {
> >> +error_setg(errp, "New snapshot node name already exists");
> >> +return;
> >> +}
> >
> > Preexisting, but shouldn't we use bdrv_lookup_bs() here (because devices
> > and node names share a namespace)?
> 
> I think you're right, good catch!
> 
> >> +if (state->new_bs->blk != NULL) {
> >> +error_setg(errp, "The snapshot is already in use by %s",
> >> +   blk_name(state->new_bs->blk));
> >> +return;
> >> +}
> >
> > Is it even possible yet to create a root BDS without a BB?
> 
> It is possible with Max's series, on which mine depends.
> 
>http://patchwork.ozlabs.org/patch/519375/

Okay. I missed this dependency, it doesn't seem to be very explicit in
the cover letter.

> >> +if (bdrv_op_is_blocked(state->new_bs, BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT,
> >> +   errp)) {
> >> +return;
> >> +}
> >> +
> >> +if (state->new_bs->backing_hd != NULL) {
> >> +error_setg(errp, "The snapshot already has a backing image");
> >>  }
> >
> > The error cases after bdrv_open() should probably bdrv_unref() the
> > node.
> 
> I don't think it's necessary, external_snapshot_abort() already takes
> care of that.

Sorry for the noise, you're right. I was confused by bdrv_reopen()
transactions working differently: There, abort isn't called for the
queue entry that has failed prepare, but here it is.

Kevin

Re: [Qemu-devel] [PATCH v6 3/4] block: add a 'blockdev-snapshot' QMP command

[Alberto Garcia]

On Tue 06 Oct 2015 05:30:07 PM CEST, Kevin Wolf wrote:
>> -options = qdict_new();
>> -if (has_snapshot_node_name) {
>> -qdict_put(options, "node-name",
>> -  qstring_from_str(snapshot_node_name));
>> +if (snapshot_node_name && bdrv_find_node(snapshot_node_name)) {
>> +error_setg(errp, "New snapshot node name already exists");
>> +return;
>> +}
>
> Preexisting, but shouldn't we use bdrv_lookup_bs() here (because devices
> and node names share a namespace)?

I think you're right, good catch!

>> +if (state->new_bs->blk != NULL) {
>> +error_setg(errp, "The snapshot is already in use by %s",
>> +   blk_name(state->new_bs->blk));
>> +return;
>> +}
>
> Is it even possible yet to create a root BDS without a BB?

It is possible with Max's series, on which mine depends.

   http://patchwork.ozlabs.org/patch/519375/

>> +if (bdrv_op_is_blocked(state->new_bs, BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT,
>> +   errp)) {
>> +return;
>> +}
>> +
>> +if (state->new_bs->backing_hd != NULL) {
>> +error_setg(errp, "The snapshot already has a backing image");
>>  }
>
> The error cases after bdrv_open() should probably bdrv_unref() the
> node.

I don't think it's necessary, external_snapshot_abort() already takes
care of that.

Thanks for reviewing!

Berto

Re: [Qemu-devel] [PATCH v6 3/4] block: add a 'blockdev-snapshot' QMP command

[Kevin Wolf]

Am 22.09.2015 um 15:28 hat Alberto Garcia geschrieben:
> 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.
> 
> 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.
> 
> 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.
> 
> 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.
> 
> Signed-off-by: Alberto Garcia 
> Cc: Eric Blake 
> Cc: Max Reitz 
> ---
>  blockdev.c   | 163 
> ---
>  qapi-schema.json |   2 +
>  qapi/block-core.json |  28 +
>  qmp-commands.hx  |  38 
>  4 files changed, 171 insertions(+), 60 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 1a5b889..daf72f3 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1183,6 +1183,18 @@ void qmp_blockdev_snapshot_sync(bool has_device, const 
> char *device,
> , errp);
>  }
>  
> +void qmp_blockdev_snapshot(const char *node, const char *overlay,
> +   Error **errp)
> +{
> +BlockdevSnapshot snapshot_data = {
> +.node = (char *) node,
> +.overlay = (char *) overlay
> +};
> +
> +blockdev_do_action(TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT,
> +   _data, errp);
> +}
> +
>  void qmp_blockdev_snapshot_internal_sync(const char *device,
>   const char *name,
>   Error **errp)
> @@ -1521,57 +1533,48 @@ typedef struct ExternalSnapshotState {
>  static void external_snapshot_prepare(BlkTransactionState *common,
>Error **errp)
>  {
> -int flags, ret;
> -QDict *options;
> +int flags = 0, ret;
> +QDict *options = NULL;
>  Error *local_err = NULL;
> -bool has_device = false;
> +/* Device and node name of the image to generate the snapshot from */
>  const char *device;
> -bool has_node_name = false;
>  const char *node_name;
> -bool has_snapshot_node_name = false;
> -const char *snapshot_node_name;
> +/* Reference to the new image (for 'blockdev-snapshot') */
> +const char *snapshot_ref;
> +/* File name of the new image (for 'blockdev-snapshot-sync') */
>  const char *new_image_file;
> -const char *format = "qcow2";
> -enum NewImageMode mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
>  ExternalSnapshotState *state =
>   DO_UPCAST(ExternalSnapshotState, common, 
> common);
>  TransactionAction *action = common->action;
>  
> -/* get parameters */
> -g_assert(action->kind == TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC);
> -
> -has_device = action->blockdev_snapshot_sync->has_device;
> -device = action->blockdev_snapshot_sync->device;
> -has_node_name = action->blockdev_snapshot_sync->has_node_name;
> -node_name = action->blockdev_snapshot_sync->node_name;
> -has_snapshot_node_name =
> -action->blockdev_snapshot_sync->has_snapshot_node_name;
> -snapshot_node_name = action->blockdev_snapshot_sync->snapshot_node_name;
> -
> -new_image_file = action->blockdev_snapshot_sync->snapshot_file;
> -if (action->blockdev_snapshot_sync->has_format) {
> -format = action->blockdev_snapshot_sync->format;
> -}
> -if (action->blockdev_snapshot_sync->has_mode) {
> -mode = action->blockdev_snapshot_sync->mode;
> +/* 'blockdev-snapshot' and 'blockdev-snapshot-sync' have similar
> + * purpose but a different set of parameters */
> +switch (action->kind) {
> +case TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT:
> +{
> +BlockdevSnapshot *s = action->blockdev_snapshot;
> +device = s->node;
> +node_name = s->node;
> +new_image_file = NULL;
> +snapshot_ref = s->overlay;
> +}
> +break;
> +case TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC:
> +{
> +BlockdevSnapshotSync *s = action->blockdev_snapshot_sync;
> +device = s->has_device ? s->device : NULL;
> +node_name = s->has_node_name ? s->node_name : NULL;
> +new_image_file = s->snapshot_file;
> +snapshot_ref = NULL;
> +}
> +break;
> +default:
> +g_assert_not_reached();
>  }
>  
>  /* start processing */
> -state->old_bs = bdrv_lookup_bs(has_device ? 

Re: [Qemu-devel] [PATCH v6 3/4] block: add a 'blockdev-snapshot' QMP command

[Alberto Garcia]

On Tue 22 Sep 2015 06:38:27 PM CEST, Max Reitz wrote:

>> +.name   = "blockdev-snapshot",
>> +.args_type  = "node:s,overlay:s",
>> +.mhandler.cmd_new = qmp_marshal_input_blockdev_snapshot,
>
> As of 7fad30f06eb6aa57aaa8f3d264288f24ae7646f0, this needs to be
> qmp_marshal_blockdev_snapshot.

Sure, that needs to be part of the next rebase, otherwise it won't
build.

Berto

Re: [Qemu-devel] [PATCH v6 3/4] block: add a 'blockdev-snapshot' QMP command

[Max Reitz]

On 22.09.2015 15:28, 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.
> 
> 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.
> 
> 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.
> 
> 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.
> 
> Signed-off-by: Alberto Garcia 
> Cc: Eric Blake 
> Cc: Max Reitz 
> ---
>  blockdev.c   | 163 
> ---
>  qapi-schema.json |   2 +
>  qapi/block-core.json |  28 +
>  qmp-commands.hx  |  38 
>  4 files changed, 171 insertions(+), 60 deletions(-)
> 

[snip]

> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 495670b..e5bd0e0 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -1454,6 +1454,44 @@ Example:
>  EQMP
>  
>  {
> +.name   = "blockdev-snapshot",
> +.args_type  = "node:s,overlay:s",
> +.mhandler.cmd_new = qmp_marshal_input_blockdev_snapshot,

As of 7fad30f06eb6aa57aaa8f3d264288f24ae7646f0, this needs to be
qmp_marshal_blockdev_snapshot.

> +},
> +
> +SQMP
> +blockdev-snapshot
> +-
> +Since 2.5
> +
> +Create a snapshot, by installing 'node' as the backing image of
> +'overlay'. Additionally, if 'node' is associated with a block
> +device, the block device changes to using 'overlay' as its new active
> +image.
> +
> +Arguments:
> +
> +- "node": device that will have a snapshot created (json-string)
> +- "overlay": device that will have 'node' as its backing image (json-string)
> +
> +Example:
> +
> +-> { "execute": "blockdev-add",
> +"arguments": { "options": { "driver": "qcow2",
> +"node-name": "node1534",
> +"file": { "driver": "file",
> +  "filename": 
> "hd1.qcow2" },
> +"backing": "" } } }
> +
> +<- { "return": {} }
> +
> +-> { "execute": "blockdev-snapshot", "arguments": { "node": "ide-hd0",
> +"overlay": "node1534" } }
> +<- { "return": {} }
> +
> +EQMP
> +
> +{
>  .name   = "blockdev-snapshot-internal-sync",
>  .args_type  = "device:B,name:s",
>  .mhandler.cmd_new = 
> qmp_marshal_input_blockdev_snapshot_internal_sync,

Consequently, this context needs to be fixed up, too.

With that changed:

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PATCH v6 3/4] block: add a 'blockdev-snapshot' QMP command

[Alberto Garcia]

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.

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.

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.

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.

Signed-off-by: Alberto Garcia 
Cc: Eric Blake 
Cc: Max Reitz 
---
 blockdev.c   | 163 ---
 qapi-schema.json |   2 +
 qapi/block-core.json |  28 +
 qmp-commands.hx  |  38 
 4 files changed, 171 insertions(+), 60 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 1a5b889..daf72f3 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1183,6 +1183,18 @@ void qmp_blockdev_snapshot_sync(bool has_device, const 
char *device,
, errp);
 }
 
+void qmp_blockdev_snapshot(const char *node, const char *overlay,
+   Error **errp)
+{
+BlockdevSnapshot snapshot_data = {
+.node = (char *) node,
+.overlay = (char *) overlay
+};
+
+blockdev_do_action(TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT,
+   _data, errp);
+}
+
 void qmp_blockdev_snapshot_internal_sync(const char *device,
  const char *name,
  Error **errp)
@@ -1521,57 +1533,48 @@ typedef struct ExternalSnapshotState {
 static void external_snapshot_prepare(BlkTransactionState *common,
   Error **errp)
 {
-int flags, ret;
-QDict *options;
+int flags = 0, ret;
+QDict *options = NULL;
 Error *local_err = NULL;
-bool has_device = false;
+/* Device and node name of the image to generate the snapshot from */
 const char *device;
-bool has_node_name = false;
 const char *node_name;
-bool has_snapshot_node_name = false;
-const char *snapshot_node_name;
+/* Reference to the new image (for 'blockdev-snapshot') */
+const char *snapshot_ref;
+/* File name of the new image (for 'blockdev-snapshot-sync') */
 const char *new_image_file;
-const char *format = "qcow2";
-enum NewImageMode mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
 ExternalSnapshotState *state =
  DO_UPCAST(ExternalSnapshotState, common, common);
 TransactionAction *action = common->action;
 
-/* get parameters */
-g_assert(action->kind == TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC);
-
-has_device = action->blockdev_snapshot_sync->has_device;
-device = action->blockdev_snapshot_sync->device;
-has_node_name = action->blockdev_snapshot_sync->has_node_name;
-node_name = action->blockdev_snapshot_sync->node_name;
-has_snapshot_node_name =
-action->blockdev_snapshot_sync->has_snapshot_node_name;
-snapshot_node_name = action->blockdev_snapshot_sync->snapshot_node_name;
-
-new_image_file = action->blockdev_snapshot_sync->snapshot_file;
-if (action->blockdev_snapshot_sync->has_format) {
-format = action->blockdev_snapshot_sync->format;
-}
-if (action->blockdev_snapshot_sync->has_mode) {
-mode = action->blockdev_snapshot_sync->mode;
+/* 'blockdev-snapshot' and 'blockdev-snapshot-sync' have similar
+ * purpose but a different set of parameters */
+switch (action->kind) {
+case TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT:
+{
+BlockdevSnapshot *s = action->blockdev_snapshot;
+device = s->node;
+node_name = s->node;
+new_image_file = NULL;
+snapshot_ref = s->overlay;
+}
+break;
+case TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC:
+{
+BlockdevSnapshotSync *s = action->blockdev_snapshot_sync;
+device = s->has_device ? s->device : NULL;
+node_name = s->has_node_name ? s->node_name : NULL;
+new_image_file = s->snapshot_file;
+snapshot_ref = NULL;
+}
+break;
+default:
+g_assert_not_reached();
 }
 
 /* start processing */
-state->old_bs = bdrv_lookup_bs(has_device ? device : NULL,
-   has_node_name ? node_name : NULL,
-   _err);
-if (local_err) {
-error_propagate(errp, local_err);
-return;
-}
-
-if (has_node_name && !has_snapshot_node_name) {
-error_setg(errp, "New