Patchwork [3/3] Add nocreate option to snapshot_blkdev

login
register
mail settings
Submitter Federico Simoncelli
Date Feb. 22, 2012, 5:13 p.m.
Message ID <1329930815-7995-4-git-send-email-fsimonce@redhat.com>
Download mbox | patch
Permalink /patch/142508/
State New
Headers show

Comments

Federico Simoncelli - Feb. 22, 2012, 5:13 p.m.
Signed-off-by: Federico Simoncelli <fsimonce@redhat.com>
---
 blockdev.c       |   14 ++++++++------
 hmp-commands.hx  |   16 ++++++++++------
 hmp.c            |    4 +++-
 qapi-schema.json |    8 +++++++-
 qmp-commands.hx  |    2 +-
 5 files changed, 29 insertions(+), 15 deletions(-)
Paolo Bonzini - Feb. 23, 2012, 7:19 a.m.
On 02/22/2012 06:13 PM, Federico Simoncelli wrote:
> Signed-off-by: Federico Simoncelli <fsimonce@redhat.com>

What is the usecase, and how can this be tested?

Paolo
Paolo Bonzini - Feb. 23, 2012, 7:38 a.m.
On 02/23/2012 08:19 AM, Paolo Bonzini wrote:
>> > Signed-off-by: Federico Simoncelli <fsimonce@redhat.com>
> What is the usecase, and how can this be tested?

Oops, you explained it in part 0.

> Step 5 - Final Switch to Destination
> ====================================
> VM1 is now able to switch to the destination for both read and write
> operations.
> 
> [src/hd0base] <- [src/hd0snap1] <= VM1(read-write)
> 
> (qemu) snapshot_blkdev -n ide0-hd0 /tmp/dst/hd0snap1.qcow2

It seems to me that reusing the snapshot_blkdev command is a bit of a
misnomer.  It also forces you to use format autodetection for the
destination in the blkmirror phase.

I see two possibilities:

1) you can make a new command block_reopen; this fixes only the naming
problem.

2) you can hide the new format behind a new command to be used like this:

   block_migrate --mirror ide-hd0 /tmp/dst/hd0snap1.qcow2
   ...
   block_migrate --switch ide-hd0

This leave the possibility to specify the format in the future:

   block_migrate --mirror ide-hd0 /tmp/dst/hd0snap1.qcow2 qcow2

And we could have another sub-command

   block_mirror --stream ide-hd0 /tmp/dst/hd0.raw

to migrate block devices without shared storage and without an
intermediate snapshot.

Paolo
Federico Simoncelli - Feb. 23, 2012, 9:39 a.m.
----- Original Message -----
> From: "Paolo Bonzini" <pbonzini@redhat.com>
> Cc: "Federico Simoncelli" <fsimonce@redhat.com>, kwolf@redhat.com, mtosatti@redhat.com, qemu-devel@nongnu.org
> Sent: Thursday, February 23, 2012 8:38:55 AM
> Subject: Re: [PATCH 3/3] Add nocreate option to snapshot_blkdev
> 
> On 02/23/2012 08:19 AM, Paolo Bonzini wrote:
> >> > Signed-off-by: Federico Simoncelli <fsimonce@redhat.com>
> > What is the usecase, and how can this be tested?
> 
> Oops, you explained it in part 0.
> 
> > Step 5 - Final Switch to Destination
> > ====================================
> > VM1 is now able to switch to the destination for both read and
> > write
> > operations.
> > 
> > [src/hd0base] <- [src/hd0snap1] <= VM1(read-write)
> > 
> > (qemu) snapshot_blkdev -n ide0-hd0 /tmp/dst/hd0snap1.qcow2
> 
> It seems to me that reusing the snapshot_blkdev command is a bit of a
> misnomer.  It also forces you to use format autodetection for the
> destination in the blkmirror phase.
> 
> I see two possibilities:
> 
> 1) you can make a new command block_reopen; this fixes only the
> naming
> problem.
> 
> 2) you can hide the new format behind a new command to be used like
> this:
> 
>    block_migrate --mirror ide-hd0 /tmp/dst/hd0snap1.qcow2
>    ...
>    block_migrate --switch ide-hd0
> 
> This leave the possibility to specify the format in the future:
> 
>    block_migrate --mirror ide-hd0 /tmp/dst/hd0snap1.qcow2 qcow2
> 
> And we could have another sub-command
> 
>    block_mirror --stream ide-hd0 /tmp/dst/hd0.raw
> 
> to migrate block devices without shared storage and without an
> intermediate snapshot.

I agree on the fact that using snapshot_blkdev is a misnomer but at
the same time I like very much how blkmirror is modular and it can
be used as a regular image filename.
I'm worried that making it explicit with a "--mirror" option would
take away its flexibility.
What about adding block_reopen and extending the blkmirror string:

 (qemu) block_reopen ide-hd0 \
 blkmirror:qcow2:/tmp/src/hd0snap1.qcow2:/tmp/dst/hd0snap1.qcow2

so that the format is passed as first argument (and if it's empty we
would use the auto-detection).
Paolo Bonzini - Feb. 23, 2012, 9:48 a.m.
On 02/23/2012 10:39 AM, Federico Simoncelli wrote:
> I agree on the fact that using snapshot_blkdev is a misnomer but at
> the same time I like very much how blkmirror is modular and it can
> be used as a regular image filename.

True, on the other hand if we add this we will have to keep it forever.

I'm worried that blkmirror does not satisfy _all_ mirroring needs, for
example you cannot use block_stream to copy from the source to the
destination, so perhaps in the future we want to change it to something
else.

So while I appreciate the easier debugging, I'm afraid that we'll regret
adding a command-line-visible feature.

> I'm worried that making it explicit with a "--mirror" option would
> take away its flexibility.
> What about adding block_reopen and extending the blkmirror string:
> 
>  (qemu) block_reopen ide-hd0 \
>  blkmirror:qcow2:/tmp/src/hd0snap1.qcow2:/tmp/dst/hd0snap1.qcow2
> 
> so that the format is passed as first argument (and if it's empty we
> would use the auto-detection).

No, the format of the source and destination should be independent.  It
would be nice to have something like this:

blkmirror:src=...,dst=...,srcformat=...,dstformat=...

but commas are a pain because they need escaping.

Paolo
Federico Simoncelli - Feb. 23, 2012, 10:19 a.m.
----- Original Message -----
> From: "Paolo Bonzini" <pbonzini@redhat.com>
> To: "Federico Simoncelli" <fsimonce@redhat.com>
> Cc: kwolf@redhat.com, mtosatti@redhat.com, qemu-devel@nongnu.org
> Sent: Thursday, February 23, 2012 10:48:59 AM
> Subject: Re: [PATCH 3/3] Add nocreate option to snapshot_blkdev
> 
> On 02/23/2012 10:39 AM, Federico Simoncelli wrote:
> > I agree on the fact that using snapshot_blkdev is a misnomer but at
> > the same time I like very much how blkmirror is modular and it can
> > be used as a regular image filename.
> 
> True, on the other hand if we add this we will have to keep it
> forever.
> 
> I'm worried that blkmirror does not satisfy _all_ mirroring needs,
> for
> example you cannot use block_stream to copy from the source to the
> destination, so perhaps in the future we want to change it to
> something
> else.

Are you talking about a mirroring where you block_stream the missing
clusters in the destination from the source?
I believe that it could be done without losing the blkmirror
modularity probably extending the BlockDriver structure with some
additional concepts.

> So while I appreciate the easier debugging, I'm afraid that we'll
> regret
> adding a command-line-visible feature.
> 
> > I'm worried that making it explicit with a "--mirror" option would
> > take away its flexibility.
> > What about adding block_reopen and extending the blkmirror string:
> > 
> >  (qemu) block_reopen ide-hd0 \
> >  blkmirror:qcow2:/tmp/src/hd0snap1.qcow2:/tmp/dst/hd0snap1.qcow2
> > 
> > so that the format is passed as first argument (and if it's empty
> > we
> > would use the auto-detection).
> 
> No, the format of the source and destination should be independent.
>  It
> would be nice to have something like this:
> 
> blkmirror:src=...,dst=...,srcformat=...,dstformat=...
> 
> but commas are a pain because they need escaping.

I like that, one limitation we need to keep in mind is that it should
fit into the hard-coded filename limit of 1024 characters that (sadly) we
have in multiple places. Another thing is that at this stage the mirroring
is more an original/copy concept rather than source/destination.
What about:

blkmirror:...,copy=...,fmt=... (both images uses the same fmt)
blkmirror:...,copy=...,fmt=...,copyfmt=...

Or, eventually if you feel like that source/destination is more appropriate
for the future, then:

blkmirror:...,dst=...,fmt=...,dstfmt=...
Paolo Bonzini - Feb. 23, 2012, 11:30 a.m.
On 02/23/2012 11:19 AM, Federico Simoncelli wrote:
>>> I'm worried that blkmirror does not satisfy _all_ mirroring
>>> needs, for example you cannot use block_stream to copy from the
>>> source to the destination, so perhaps in the future we want to
>>> change it to something else.
> 
> Are you talking about a mirroring where you block_stream the missing 
> clusters in the destination from the source? I believe that it could
> be done without losing the blkmirror modularity probably extending
> the BlockDriver structure with some additional concepts.

Yes, agreed.  It's not the idea of a driver that I don't like; it is
making it available through the command-line that worries me.  This
would be unlike all other "filtering" options (copy-on-read, I/O limits,
dirty sector tracking).  True, there are blkdebug and blkverify but
those are for debugging only.

Paolo

Patch

diff --git a/blockdev.c b/blockdev.c
index 7a6613a..2c3e10a 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -647,7 +647,7 @@  void do_commit(Monitor *mon, const QDict *qdict)
 
 void qmp_blockdev_snapshot_sync(const char *device, const char *snapshot_file,
                                 bool has_format, const char *format,
-                                Error **errp)
+                                bool has_nocreate, bool nocreate, Error **errp)
 {
     BlockDriverState *bs;
     BlockDriver *drv, *old_drv, *proto_drv;
@@ -686,11 +686,13 @@  void qmp_blockdev_snapshot_sync(const char *device, const char *snapshot_file,
         return;
     }
 
-    ret = bdrv_img_create(snapshot_file, format, bs->filename,
-                          bs->drv->format_name, NULL, -1, flags);
-    if (ret) {
-        error_set(errp, QERR_UNDEFINED_ERROR);
-        return;
+    if (!(has_nocreate && nocreate)) {
+        ret = bdrv_img_create(snapshot_file, format, bs->filename,
+                              bs->drv->format_name, NULL, -1, flags);
+        if (ret) {
+            error_set(errp, QERR_UNDEFINED_ERROR);
+            return;
+        }
     }
 
     bdrv_drain_all();
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 573b823..ff49412 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -868,14 +868,18 @@  ETEXI
 
     {
         .name       = "snapshot_blkdev",
-        .args_type  = "device:B,snapshot-file:s?,format:s?",
-        .params     = "device [new-image-file] [format]",
+        .args_type  = "nocreate:-n,device:B,snapshot-file:s?,format:s?",
+        .params     = "[-n] device [new-image-file] [format]",
         .help       = "initiates a live snapshot\n\t\t\t"
                       "of device. If a new image file is specified, the\n\t\t\t"
-                      "new image file will become the new root image.\n\t\t\t"
-                      "If format is specified, the snapshot file will\n\t\t\t"
-                      "be created in that format. Otherwise the\n\t\t\t"
-                      "snapshot will be internal! (currently unsupported)",
+                      "new image file will be created (unless -n is\n\t\t\t"
+                      "specified) and will become the new root image.\n\t\t\t"
+                      "The -n (nocreate) option is potentially dangerous\n\t\t\t"
+                      "if the image wasn't prepared in the correct way\n\t\t\t"
+                      "by an external manager. If format is specified,\n\t\t\t"
+                      "the snapshot file will be created in that format.\n\t\t\t"
+                      "Otherwise the snapshot will be internal!\n\t\t\t"
+                      "(currently unsupported)",
         .mhandler.cmd = hmp_snapshot_blkdev,
     },
 
diff --git a/hmp.c b/hmp.c
index 8ff8c94..44868c7 100644
--- a/hmp.c
+++ b/hmp.c
@@ -684,6 +684,7 @@  void hmp_block_resize(Monitor *mon, const QDict *qdict)
 
 void hmp_snapshot_blkdev(Monitor *mon, const QDict *qdict)
 {
+    int nocreate = qdict_get_try_bool(qdict, "nocreate", 0);
     const char *device = qdict_get_str(qdict, "device");
     const char *filename = qdict_get_try_str(qdict, "snapshot-file");
     const char *format = qdict_get_try_str(qdict, "format");
@@ -697,7 +698,8 @@  void hmp_snapshot_blkdev(Monitor *mon, const QDict *qdict)
         return;
     }
 
-    qmp_blockdev_snapshot_sync(device, filename, !!format, format, &errp);
+    qmp_blockdev_snapshot_sync(device, filename,
+                               !!format, format, true, nocreate, &errp);
     hmp_handle_error(mon, &errp);
 }
 
diff --git a/qapi-schema.json b/qapi-schema.json
index d02ee86..ac652c2 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1116,9 +1116,14 @@ 
 # @snapshot-file: the target of the new image. If the file exists, or if it
 #                 is a device, the snapshot will be created in the existing
 #                 file/device. If does not exist, a new file will be created.
+#                 This behavior can be overridden using the nocreate option.
 #
 # @format: #optional the format of the snapshot image, default is 'qcow2'.
 #
+# @nocreate: #optional use the target image avoiding the creation. This is
+#            a potentially dangerous option if the image wasn't prepared
+#            in the correct way by an external manager.
+#
 # Returns: nothing on success
 #          If @device is not a valid block device, DeviceNotFound
 #          If @snapshot-file can't be opened, OpenFileFailed
@@ -1133,7 +1138,8 @@ 
 # Since 0.14.0
 ##
 { 'command': 'blockdev-snapshot-sync',
-  'data': { 'device': 'str', 'snapshot-file': 'str', '*format': 'str' } }
+  'data': { 'device': 'str', 'snapshot-file': 'str', '*format': 'str',
+            '*nocreate': 'bool' } }
 
 ##
 # @human-monitor-command:
diff --git a/qmp-commands.hx b/qmp-commands.hx
index b5e2ab8..e717826 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -668,7 +668,7 @@  EQMP
 
     {
         .name       = "blockdev-snapshot-sync",
-        .args_type  = "device:B,snapshot-file:s,format:s?",
+        .args_type  = "nocreate:-n,device:B,snapshot-file:s,format:s?",
         .mhandler.cmd_new = qmp_marshal_input_blockdev_snapshot_sync,
     },