[Qemu-devel] [PATCH V3 0/7] export internal snapshot by qemu-nbd

[Qemu-devel] [PATCH V3 0/7] export internal snapshot by qemu-nbd

[Wenchao Xia]

This series allow user to read internal snapshot's contents without qemu-img
convert.

V2:
  Address Stefan's comments:
  02: add 'fall through' comments in the case statement.
  03: add doc about the difference of internal snapshot and backing chain
snapshot, which is used in previous '--snapshot' parameter.
  Other:
  01,04: rebased on upstream with conflict resolved.

v3:
  Address Paolo's comments:
  02: add parameter "-l snapshot_id_or_name", rename options
snapshot-load to load-snapshot, use QemuOpts.
  03: rename snapshot-load to load-snapshot.
  04: related change to test both -l and -L case.
  05-07: add similar parameter for qemu-img convert.
  Other:
  01: foldered original snapshot logic into function
bdrv_snapshot_load_tmp_by_id_or_name(), since multiple
caller present in this version. Refined error message from
", reason: %s" to ": %s".
  02: Refined error message from ", reason: %s" to ": %s".
  03: Rename PARAM to SNAPSHOT_PARAM.

Wenchao Xia (7):
  1 snapshot: distinguish id and name in load_tmp
  2 qemu-nbd: support internal snapshot export
  3 qemu-nbd: add doc for internal snapshot export
  4 qemu-iotests: add 058 internal snapshot export with qemu-nbd case
  5 qemu-img: add -L for snapshot in convert
  6 qemu-img: add doc for param -L in convert
  7 qemu-iotests: add test for snapshot in qemu-img convert

 block/qcow2-snapshot.c     |   16 +++++-
 block/qcow2.h              |    5 ++-
 block/snapshot.c           |   76 +++++++++++++++++++++++++++-
 include/block/block_int.h  |    4 +-
 include/block/snapshot.h   |   13 ++++-
 qemu-img-cmds.hx           |    2 +-
 qemu-img.c                 |   32 +++++++++---
 qemu-img.texi              |    7 ++-
 qemu-nbd.c                 |   46 ++++++++++++++++-
 qemu-nbd.texi              |   11 ++++-
 tests/qemu-iotests/058     |  121 ++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/058.out |   44 ++++++++++++++++
 tests/qemu-iotests/check   |    1 +
 tests/qemu-iotests/group   |    1 +
 14 files changed, 359 insertions(+), 20 deletions(-)
 create mode 100755 tests/qemu-iotests/058
 create mode 100644 tests/qemu-iotests/058.out

[Qemu-devel] [PATCH V3 1/7] snapshot: distinguish id and name in load_tmp

[Wenchao Xia]

Since later this function will be used so improve it. The only caller of it
now is qemu-img, and it is not impacted by introduce function
bdrv_snapshot_load_tmp_by_id_or_name() that call bdrv_snapshot_load_tmp()
twice to keep old search logic. bdrv_snapshot_load_tmp_by_id_or_name() return
int to let caller know the errno, and errno will be used later.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 block/qcow2-snapshot.c    |   16 +++++++++++-
 block/qcow2.h             |    5 +++-
 block/snapshot.c          |   58 +++++++++++++++++++++++++++++++++++++++++++-
 include/block/block_int.h |    4 ++-
 include/block/snapshot.h  |    7 ++++-
 qemu-img.c                |    8 ++++-
 6 files changed, 89 insertions(+), 9 deletions(-)

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 5e8a779..4911ebe 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -669,7 +669,10 @@ int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
     return s->nb_snapshots;
 }
 
-int qcow2_snapshot_load_tmp(BlockDriverState *bs, const char *snapshot_name)
+int qcow2_snapshot_load_tmp(BlockDriverState *bs,
+                            const char *snapshot_id,
+                            const char *name,
+                            Error **errp)
 {
     int i, snapshot_index;
     BDRVQcowState *s = bs->opaque;
@@ -677,12 +680,17 @@ int qcow2_snapshot_load_tmp(BlockDriverState *bs, const char *snapshot_name)
     uint64_t *new_l1_table;
     int new_l1_bytes;
     int ret;
+    const char *device = bdrv_get_device_name(bs);
 
     assert(bs->read_only);
 
     /* Search the snapshot */
-    snapshot_index = find_snapshot_by_id_or_name(bs, snapshot_name);
+    snapshot_index = find_snapshot_by_id_and_name(bs, snapshot_id, name);
     if (snapshot_index < 0) {
+        error_setg(errp,
+                   "Can't find a snapshot with ID '%s' and name '%s' "
+                   "on device '%s'",
+                   STR_OR_NULL(snapshot_id), STR_OR_NULL(name), device);
         return -ENOENT;
     }
     sn = &s->snapshots[snapshot_index];
@@ -693,6 +701,10 @@ int qcow2_snapshot_load_tmp(BlockDriverState *bs, const char *snapshot_name)
 
     ret = bdrv_pread(bs->file, sn->l1_table_offset, new_l1_table, new_l1_bytes);
     if (ret < 0) {
+        error_setg(errp,
+                   "Failed to read l1 table for snapshot with ID '%s' and name "
+                   "'%s' on device '%s'",
+                   sn->id_str, sn->name, device);
         g_free(new_l1_table);
         return ret;
     }
diff --git a/block/qcow2.h b/block/qcow2.h
index c90e5d6..12cfeaf 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -472,7 +472,10 @@ int qcow2_snapshot_delete(BlockDriverState *bs,
                           const char *name,
                           Error **errp);
 int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab);
-int qcow2_snapshot_load_tmp(BlockDriverState *bs, const char *snapshot_name);
+int qcow2_snapshot_load_tmp(BlockDriverState *bs,
+                            const char *snapshot_id,
+                            const char *name,
+                            Error **errp);
 
 void qcow2_free_snapshots(BlockDriverState *bs);
 int qcow2_read_snapshots(BlockDriverState *bs);
diff --git a/block/snapshot.c b/block/snapshot.c
index a05c0c0..2ae3099 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -265,18 +265,72 @@ int bdrv_snapshot_list(BlockDriverState *bs,
     return -ENOTSUP;
 }
 
+/**
+ * Temporarily load an internal snapshot by @snapshot_id and @name.
+ * @bs: block device used in the operation
+ * @snapshot_id: unique snapshot ID, or NULL
+ * @name: snapshot name, or NULL
+ * @errp: location to store error
+ *
+ * If both @snapshot_id and @name are specified, load the first one with
+ * id @snapshot_id and name @name.
+ * If only @snapshot_id is specified, load the first one with id
+ * @snapshot_id.
+ * If only @name is specified, load the first one with name @name.
+ * if none is specified, return -ENINVAL.
+ *
+ * Returns: 0 on success, -errno on fail. If @bs is not inserted, return
+ * -ENOMEDIUM. If @bs is not readonly, return -EINVAL. If @bs did not support
+ * internal snapshot, return -ENOTSUP. If qemu can't find one matching @id and
+ * @name, return -ENOENT. If @bs do not support parameter @snapshot_id or
+ * @name, return -EINVAL. If @errp != NULL, it will always be filled on
+ * failure.
+ */
 int bdrv_snapshot_load_tmp(BlockDriverState *bs,
-        const char *snapshot_name)
+                           const char *snapshot_id,
+                           const char *name,
+                           Error **errp)
 {
     BlockDriver *drv = bs->drv;
+    const char *device = bdrv_get_device_name(bs);
     if (!drv) {
+        error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
         return -ENOMEDIUM;
     }
+    if (!snapshot_id && !name) {
+        error_setg(errp, "snapshot_id and name are both NULL");
+        return -EINVAL;
+    }
     if (!bs->read_only) {
+        error_setg(errp, "Device '%s' is not readonly", device);
         return -EINVAL;
     }
     if (drv->bdrv_snapshot_load_tmp) {
-        return drv->bdrv_snapshot_load_tmp(bs, snapshot_name);
+        return drv->bdrv_snapshot_load_tmp(bs, snapshot_id, name, errp);
     }
+    error_set(errp, QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,
+              drv->format_name, device,
+              "temporarily load internal snapshot");
     return -ENOTSUP;
 }
+
+int bdrv_snapshot_load_tmp_by_id_or_name(BlockDriverState *bs,
+                                         const char *id_or_name,
+                                         Error **errp)
+{
+    int ret;
+    Error *local_err = NULL;
+
+    ret = bdrv_snapshot_load_tmp(bs, id_or_name, NULL, &local_err);
+    if (ret == -ENOENT || ret == -EINVAL) {
+        error_free(local_err);
+        local_err = NULL;
+        ret = bdrv_snapshot_load_tmp(bs, NULL, id_or_name, &local_err);
+    }
+
+    if (error_is_set(&local_err)) {
+        error_propagate(errp, local_err);
+    }
+
+    return ret;
+}
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 3eeb6fe..554cadd 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -160,7 +160,9 @@ struct BlockDriver {
     int (*bdrv_snapshot_list)(BlockDriverState *bs,
                               QEMUSnapshotInfo **psn_info);
     int (*bdrv_snapshot_load_tmp)(BlockDriverState *bs,
-                                  const char *snapshot_name);
+                                  const char *snapshot_id,
+                                  const char *name,
+                                  Error **errp);
     int (*bdrv_get_info)(BlockDriverState *bs, BlockDriverInfo *bdi);
 
     int (*bdrv_save_vmstate)(BlockDriverState *bs, QEMUIOVector *qiov,
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index 012bf22..d05bea7 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -61,5 +61,10 @@ void bdrv_snapshot_delete_by_id_or_name(BlockDriverState *bs,
 int bdrv_snapshot_list(BlockDriverState *bs,
                        QEMUSnapshotInfo **psn_info);
 int bdrv_snapshot_load_tmp(BlockDriverState *bs,
-                           const char *snapshot_name);
+                           const char *snapshot_id,
+                           const char *name,
+                           Error **errp);
+int bdrv_snapshot_load_tmp_by_id_or_name(BlockDriverState *bs,
+                                         const char *id_or_name,
+                                         Error **errp);
 #endif
diff --git a/qemu-img.c b/qemu-img.c
index 926f0a0..6df58ed 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1260,8 +1260,12 @@ static int img_convert(int argc, char **argv)
             ret = -1;
             goto out;
         }
-        if (bdrv_snapshot_load_tmp(bs[0], snapshot_name) < 0) {
-            error_report("Failed to load snapshot");
+
+        bdrv_snapshot_load_tmp_by_id_or_name(bs[0], snapshot_name, &local_err);
+        if (error_is_set(&local_err)) {
+            error_report("Failed to load snapshot: %s",
+                         error_get_pretty(local_err));
+            error_free(local_err);
             ret = -1;
             goto out;
         }
-- 
1.7.1

Re: [Qemu-devel] [PATCH V3 1/7] snapshot: distinguish id and name in load_tmp

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1647 bytes --]

On 09/25/2013 06:16 PM, Wenchao Xia wrote:
> Since later this function will be used so improve it. The only caller of it
> now is qemu-img, and it is not impacted by introduce function
> bdrv_snapshot_load_tmp_by_id_or_name() that call bdrv_snapshot_load_tmp()
> twice to keep old search logic. bdrv_snapshot_load_tmp_by_id_or_name() return
> int to let caller know the errno, and errno will be used later.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  block/qcow2-snapshot.c    |   16 +++++++++++-
>  block/qcow2.h             |    5 +++-
>  block/snapshot.c          |   58 +++++++++++++++++++++++++++++++++++++++++++-
>  include/block/block_int.h |    4 ++-
>  include/block/snapshot.h  |    7 ++++-
>  qemu-img.c                |    8 ++++-
>  6 files changed, 89 insertions(+), 9 deletions(-)
> 
> + *
> + * If both @snapshot_id and @name are specified, load the first one with
> + * id @snapshot_id and name @name.
> + * If only @snapshot_id is specified, load the first one with id
> + * @snapshot_id.
> + * If only @name is specified, load the first one with name @name.
> + * if none is specified, return -ENINVAL.

s/ENINVAL/EINVAL/

> + *
> + * Returns: 0 on success, -errno on fail. If @bs is not inserted, return
> + * -ENOMEDIUM. If @bs is not readonly, return -EINVAL. If @bs did not support
> + * internal snapshot, return -ENOTSUP. If qemu can't find one matching @id and
> + * @name, return -ENOENT. If @bs do not support parameter @snapshot_id or

s/do/does/

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH V3 1/7] snapshot: distinguish id and name in load_tmp

[Wenchao Xia]

于 2013/10/1 22:35, Eric Blake 写道:
> On 09/25/2013 06:16 PM, Wenchao Xia wrote:
>> Since later this function will be used so improve it. The only caller of it
>> now is qemu-img, and it is not impacted by introduce function
>> bdrv_snapshot_load_tmp_by_id_or_name() that call bdrv_snapshot_load_tmp()
>> twice to keep old search logic. bdrv_snapshot_load_tmp_by_id_or_name() return
>> int to let caller know the errno, and errno will be used later.
>>
>> Signed-off-by: Wenchao Xia<xiawenc@linux.vnet.ibm.com>
>> ---
>>   block/qcow2-snapshot.c    |   16 +++++++++++-
>>   block/qcow2.h             |    5 +++-
>>   block/snapshot.c          |   58 +++++++++++++++++++++++++++++++++++++++++++-
>>   include/block/block_int.h |    4 ++-
>>   include/block/snapshot.h  |    7 ++++-
>>   qemu-img.c                |    8 ++++-
>>   6 files changed, 89 insertions(+), 9 deletions(-)
>>
>> + *
>> + * If both @snapshot_id and @name are specified, load the first one with
>> + * id @snapshot_id and name @name.
>> + * If only @snapshot_id is specified, load the first one with id
>> + * @snapshot_id.
>> + * If only @name is specified, load the first one with name @name.
>> + * if none is specified, return -ENINVAL.
> s/ENINVAL/EINVAL/
>
>> + *
>> + * Returns: 0 on success, -errno on fail. If @bs is not inserted, return
>> + * -ENOMEDIUM. If @bs is not readonly, return -EINVAL. If @bs did not support
>> + * internal snapshot, return -ENOTSUP. If qemu can't find one matching @id and
>> + * @name, return -ENOENT. If @bs do not support parameter @snapshot_id or
> s/do/does/
>
will fix the grammar, thanks.

[Qemu-devel] [PATCH V3 2/7] qemu-nbd: support internal snapshot export

[Wenchao Xia]

Now it is possible to directly export an internal snapshot, which
can be used to probe the snapshot's contents without qemu-img
convert.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 block/snapshot.c         |   18 ++++++++++++++++++
 include/block/snapshot.h |    6 ++++++
 qemu-nbd.c               |   35 ++++++++++++++++++++++++++++++++++-
 3 files changed, 58 insertions(+), 1 deletions(-)

diff --git a/block/snapshot.c b/block/snapshot.c
index 2ae3099..b371c27 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -25,6 +25,24 @@
 #include "block/snapshot.h"
 #include "block/block_int.h"
 
+QemuOptsList internal_snapshot_opts = {
+    .name = "snapshot",
+    .head = QTAILQ_HEAD_INITIALIZER(internal_snapshot_opts.head),
+    .desc = {
+        {
+            .name = SNAPSHOT_OPT_ID,
+            .type = QEMU_OPT_STRING,
+            .help = "snapshot id"
+        },{
+            .name = SNAPSHOT_OPT_NAME,
+            .type = QEMU_OPT_STRING,
+            .help = "snapshot name"
+        },{
+            /* end of list */
+        }
+    },
+};
+
 int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
                        const char *name)
 {
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index d05bea7..c524a49 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -27,6 +27,12 @@
 
 #include "qemu-common.h"
 #include "qapi/error.h"
+#include "qemu/option.h"
+
+#define SNAPSHOT_OPT_ID         "snapshot.id"
+#define SNAPSHOT_OPT_NAME       "snapshot.name"
+
+extern QemuOptsList internal_snapshot_opts;
 
 typedef struct QEMUSnapshotInfo {
     char id_str[128]; /* unique snapshot id */
diff --git a/qemu-nbd.c b/qemu-nbd.c
index c26c98e..6588a1f 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -20,6 +20,7 @@
 #include "block/block.h"
 #include "block/nbd.h"
 #include "qemu/main-loop.h"
+#include "block/snapshot.h"
 
 #include <stdarg.h>
 #include <stdio.h>
@@ -315,7 +316,9 @@ int main(int argc, char **argv)
     char *device = NULL;
     int port = NBD_DEFAULT_PORT;
     off_t fd_size;
-    const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:t";
+    QemuOpts *sn_opts = NULL;
+    const char *sn_id_or_name = NULL;
+    const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:tl:L:";
     struct option lopt[] = {
         { "help", 0, NULL, 'h' },
         { "version", 0, NULL, 'V' },
@@ -328,6 +331,8 @@ int main(int argc, char **argv)
         { "connect", 1, NULL, 'c' },
         { "disconnect", 0, NULL, 'd' },
         { "snapshot", 0, NULL, 's' },
+        { "load-snapshot", 1, NULL, 'l' },
+        { "load-snapshot1", 1, NULL, 'L' },
         { "nocache", 0, NULL, 'n' },
         { "cache", 1, NULL, QEMU_NBD_OPT_CACHE },
 #ifdef CONFIG_LINUX_AIO
@@ -428,6 +433,14 @@ int main(int argc, char **argv)
                 errx(EXIT_FAILURE, "Offset must be positive `%s'", optarg);
             }
             break;
+        case 'l':
+            sn_id_or_name = optarg;
+            nbdflags |= NBD_FLAG_READ_ONLY;
+            flags &= ~BDRV_O_RDWR;
+            break;
+        case 'L':
+            sn_opts = qemu_opts_parse(&internal_snapshot_opts, optarg, 0);
+            /* fall through */
         case 'r':
             nbdflags |= NBD_FLAG_READ_ONLY;
             flags &= ~BDRV_O_RDWR;
@@ -581,6 +594,22 @@ int main(int argc, char **argv)
             error_get_pretty(local_err));
     }
 
+    if (sn_opts) {
+        ret = bdrv_snapshot_load_tmp(bs,
+                                     qemu_opt_get(sn_opts, SNAPSHOT_OPT_ID),
+                                     qemu_opt_get(sn_opts, SNAPSHOT_OPT_NAME),
+                                     &local_err);
+    } else if (sn_id_or_name) {
+        ret = bdrv_snapshot_load_tmp_by_id_or_name(bs, sn_id_or_name,
+                                                   &local_err);
+    }
+    if (ret < 0) {
+        errno = -ret;
+        err(EXIT_FAILURE,
+            "Failed to load snapshot: %s",
+            error_get_pretty(local_err));
+    }
+
     fd_size = bdrv_getlength(bs);
 
     if (partition != -1) {
@@ -641,6 +670,10 @@ int main(int argc, char **argv)
         unlink(sockpath);
     }
 
+    if (sn_opts) {
+        qemu_opts_del(sn_opts);
+    }
+
     if (device) {
         void *ret;
         pthread_join(client_thread, &ret);
-- 
1.7.1

Re: [Qemu-devel] [PATCH V3 2/7] qemu-nbd: support internal snapshot export

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1653 bytes --]

On 09/25/2013 06:16 PM, Wenchao Xia wrote:
> Now it is possible to directly export an internal snapshot, which
> can be used to probe the snapshot's contents without qemu-img
> convert.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  block/snapshot.c         |   18 ++++++++++++++++++
>  include/block/snapshot.h |    6 ++++++
>  qemu-nbd.c               |   35 ++++++++++++++++++++++++++++++++++-
>  3 files changed, 58 insertions(+), 1 deletions(-)

Woo-hoo!  Long desired.  It would be even cooler if we could set up the
export directly from a running qemu instance, rather than having to
spawn an external qemu-nbd process (after all, we've already documented
that having more than one process directly using the same qcow2 file is
not safe), but this is a step in the right direction.

> @@ -328,6 +331,8 @@ int main(int argc, char **argv)
>          { "connect", 1, NULL, 'c' },
>          { "disconnect", 0, NULL, 'd' },
>          { "snapshot", 0, NULL, 's' },
> +        { "load-snapshot", 1, NULL, 'l' },
> +        { "load-snapshot1", 1, NULL, 'L' },

Uggh.  This makes getopt_long's unambiguous-prefix handling very hard to
use.  For example, --dis is recognized as short for --disconnect; but
here --load is NOT short for --load-snapshot, because it could also be
short for --load-snapshot1.  And why the 1 suffix?  Can you come up with
a better name that distinguishes why you have to have two different long
options, and which doesn't overlap on as much of a common prefix?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH V3 2/7] qemu-nbd: support internal snapshot export

[Paolo Bonzini]

Il 26/09/2013 02:16, Wenchao Xia ha scritto:
> Now it is possible to directly export an internal snapshot, which
> can be used to probe the snapshot's contents without qemu-img
> convert.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  block/snapshot.c         |   18 ++++++++++++++++++
>  include/block/snapshot.h |    6 ++++++
>  qemu-nbd.c               |   35 ++++++++++++++++++++++++++++++++++-
>  3 files changed, 58 insertions(+), 1 deletions(-)
> 
> diff --git a/block/snapshot.c b/block/snapshot.c
> index 2ae3099..b371c27 100644
> --- a/block/snapshot.c
> +++ b/block/snapshot.c
> @@ -25,6 +25,24 @@
>  #include "block/snapshot.h"
>  #include "block/block_int.h"
>  
> +QemuOptsList internal_snapshot_opts = {
> +    .name = "snapshot",
> +    .head = QTAILQ_HEAD_INITIALIZER(internal_snapshot_opts.head),
> +    .desc = {
> +        {
> +            .name = SNAPSHOT_OPT_ID,

Why not just use "id" and "name"?

> +            .type = QEMU_OPT_STRING,
> +            .help = "snapshot id"
> +        },{
> +            .name = SNAPSHOT_OPT_NAME,
> +            .type = QEMU_OPT_STRING,
> +            .help = "snapshot name"
> +        },{
> +            /* end of list */
> +        }
> +    },
> +};
> +
>  int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
>                         const char *name)
>  {
> diff --git a/include/block/snapshot.h b/include/block/snapshot.h
> index d05bea7..c524a49 100644
> --- a/include/block/snapshot.h
> +++ b/include/block/snapshot.h
> @@ -27,6 +27,12 @@
>  
>  #include "qemu-common.h"
>  #include "qapi/error.h"
> +#include "qemu/option.h"
> +
> +#define SNAPSHOT_OPT_ID         "snapshot.id"
> +#define SNAPSHOT_OPT_NAME       "snapshot.name"
> +
> +extern QemuOptsList internal_snapshot_opts;
>  
>  typedef struct QEMUSnapshotInfo {
>      char id_str[128]; /* unique snapshot id */
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index c26c98e..6588a1f 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -20,6 +20,7 @@
>  #include "block/block.h"
>  #include "block/nbd.h"
>  #include "qemu/main-loop.h"
> +#include "block/snapshot.h"
>  
>  #include <stdarg.h>
>  #include <stdio.h>
> @@ -315,7 +316,9 @@ int main(int argc, char **argv)
>      char *device = NULL;
>      int port = NBD_DEFAULT_PORT;
>      off_t fd_size;
> -    const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:t";
> +    QemuOpts *sn_opts = NULL;
> +    const char *sn_id_or_name = NULL;
> +    const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:tl:L:";
>      struct option lopt[] = {
>          { "help", 0, NULL, 'h' },
>          { "version", 0, NULL, 'V' },
> @@ -328,6 +331,8 @@ int main(int argc, char **argv)
>          { "connect", 1, NULL, 'c' },
>          { "disconnect", 0, NULL, 'd' },
>          { "snapshot", 0, NULL, 's' },
> +        { "load-snapshot", 1, NULL, 'l' },

Just omit the long option here...

> +        { "load-snapshot1", 1, NULL, 'L' },

... and call this "load-snapshot".

Paolo

>          { "nocache", 0, NULL, 'n' },
>          { "cache", 1, NULL, QEMU_NBD_OPT_CACHE },
>  #ifdef CONFIG_LINUX_AIO
> @@ -428,6 +433,14 @@ int main(int argc, char **argv)
>                  errx(EXIT_FAILURE, "Offset must be positive `%s'", optarg);
>              }
>              break;
> +        case 'l':
> +            sn_id_or_name = optarg;
> +            nbdflags |= NBD_FLAG_READ_ONLY;
> +            flags &= ~BDRV_O_RDWR;
> +            break;
> +        case 'L':
> +            sn_opts = qemu_opts_parse(&internal_snapshot_opts, optarg, 0);
> +            /* fall through */
>          case 'r':
>              nbdflags |= NBD_FLAG_READ_ONLY;
>              flags &= ~BDRV_O_RDWR;
> @@ -581,6 +594,22 @@ int main(int argc, char **argv)
>              error_get_pretty(local_err));
>      }
>  
> +    if (sn_opts) {
> +        ret = bdrv_snapshot_load_tmp(bs,
> +                                     qemu_opt_get(sn_opts, SNAPSHOT_OPT_ID),
> +                                     qemu_opt_get(sn_opts, SNAPSHOT_OPT_NAME),
> +                                     &local_err);
> +    } else if (sn_id_or_name) {
> +        ret = bdrv_snapshot_load_tmp_by_id_or_name(bs, sn_id_or_name,
> +                                                   &local_err);
> +    }
> +    if (ret < 0) {
> +        errno = -ret;
> +        err(EXIT_FAILURE,
> +            "Failed to load snapshot: %s",
> +            error_get_pretty(local_err));
> +    }
> +
>      fd_size = bdrv_getlength(bs);
>  
>      if (partition != -1) {
> @@ -641,6 +670,10 @@ int main(int argc, char **argv)
>          unlink(sockpath);
>      }
>  
> +    if (sn_opts) {
> +        qemu_opts_del(sn_opts);
> +    }
> +
>      if (device) {
>          void *ret;
>          pthread_join(client_thread, &ret);
> 

Re: [Qemu-devel] [PATCH V3 2/7] qemu-nbd: support internal snapshot export

[Wenchao Xia]

于 2013/10/2 0:08, Paolo Bonzini 写道:
> Il 26/09/2013 02:16, Wenchao Xia ha scritto:
>> Now it is possible to directly export an internal snapshot, which
>> can be used to probe the snapshot's contents without qemu-img
>> convert.
>>
>> Signed-off-by: Wenchao Xia<xiawenc@linux.vnet.ibm.com>
>> ---
>>   block/snapshot.c         |   18 ++++++++++++++++++
>>   include/block/snapshot.h |    6 ++++++
>>   qemu-nbd.c               |   35 ++++++++++++++++++++++++++++++++++-
>>   3 files changed, 58 insertions(+), 1 deletions(-)
>>
>> diff --git a/block/snapshot.c b/block/snapshot.c
>> index 2ae3099..b371c27 100644
>> --- a/block/snapshot.c
>> +++ b/block/snapshot.c
>> @@ -25,6 +25,24 @@
>>   #include "block/snapshot.h"
>>   #include "block/block_int.h"
>>
>> +QemuOptsList internal_snapshot_opts = {
>> +    .name = "snapshot",
>> +    .head = QTAILQ_HEAD_INITIALIZER(internal_snapshot_opts.head),
>> +    .desc = {
>> +        {
>> +            .name = SNAPSHOT_OPT_ID,
> Why not just use "id" and "name"?
>
Later it is used by code:
qemu_opt_get(sn_opts, SNAPSHOT_OPT_ID),
The macro is used to avoid type it twice in the codes, shouldn't it be used?

Another reason not using "id" is because string "id" is treated as 
special case
in opts_parse() so I choosed string "snapshot.id".

>> +            .type = QEMU_OPT_STRING,
>> +            .help = "snapshot id"
>> +        },{
>> +            .name = SNAPSHOT_OPT_NAME,
>> +            .type = QEMU_OPT_STRING,
>> +            .help = "snapshot name"
>> +        },{
>> +            /* end of list */
>> +        }
>> +    },
>> +};
>> +
>>   int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
>>                          const char *name)
>>   {
>> diff --git a/include/block/snapshot.h b/include/block/snapshot.h
>> index d05bea7..c524a49 100644
>> --- a/include/block/snapshot.h
>> +++ b/include/block/snapshot.h
>> @@ -27,6 +27,12 @@
>>
>>   #include "qemu-common.h"
>>   #include "qapi/error.h"
>> +#include "qemu/option.h"
>> +
>> +#define SNAPSHOT_OPT_ID         "snapshot.id"
>> +#define SNAPSHOT_OPT_NAME       "snapshot.name"
>> +
>> +extern QemuOptsList internal_snapshot_opts;
>>
>>   typedef struct QEMUSnapshotInfo {
>>       char id_str[128]; /* unique snapshot id */
>> diff --git a/qemu-nbd.c b/qemu-nbd.c
>> index c26c98e..6588a1f 100644
>> --- a/qemu-nbd.c
>> +++ b/qemu-nbd.c
>> @@ -20,6 +20,7 @@
>>   #include "block/block.h"
>>   #include "block/nbd.h"
>>   #include "qemu/main-loop.h"
>> +#include "block/snapshot.h"
>>
>>   #include<stdarg.h>
>>   #include<stdio.h>
>> @@ -315,7 +316,9 @@ int main(int argc, char **argv)
>>       char *device = NULL;
>>       int port = NBD_DEFAULT_PORT;
>>       off_t fd_size;
>> -    const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:t";
>> +    QemuOpts *sn_opts = NULL;
>> +    const char *sn_id_or_name = NULL;
>> +    const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:tl:L:";
>>       struct option lopt[] = {
>>           { "help", 0, NULL, 'h' },
>>           { "version", 0, NULL, 'V' },
>> @@ -328,6 +331,8 @@ int main(int argc, char **argv)
>>           { "connect", 1, NULL, 'c' },
>>           { "disconnect", 0, NULL, 'd' },
>>           { "snapshot", 0, NULL, 's' },
>> +        { "load-snapshot", 1, NULL, 'l' },
> Just omit the long option here...
>
>> +        { "load-snapshot1", 1, NULL, 'L' },
> ... and call this "load-snapshot".
>
> Paolo
>
OK, I will change as:

{ NULL, 1, NULL, 'l' },

{ "load-snapshot", 1, NULL, 'L' },

>>           { "nocache", 0, NULL, 'n' },
>>           { "cache", 1, NULL, QEMU_NBD_OPT_CACHE },
>>   #ifdef CONFIG_LINUX_AIO
>> @@ -428,6 +433,14 @@ int main(int argc, char **argv)
>>                   errx(EXIT_FAILURE, "Offset must be positive `%s'", optarg);
>>               }
>>               break;
>> +        case 'l':
>> +            sn_id_or_name = optarg;
>> +            nbdflags |= NBD_FLAG_READ_ONLY;
>> +            flags&= ~BDRV_O_RDWR;
>> +            break;
>> +        case 'L':
>> +            sn_opts = qemu_opts_parse(&internal_snapshot_opts, optarg, 0);
>> +            /* fall through */
>>           case 'r':
>>               nbdflags |= NBD_FLAG_READ_ONLY;
>>               flags&= ~BDRV_O_RDWR;
>> @@ -581,6 +594,22 @@ int main(int argc, char **argv)
>>               error_get_pretty(local_err));
>>       }
>>
>> +    if (sn_opts) {
>> +        ret = bdrv_snapshot_load_tmp(bs,
>> +                                     qemu_opt_get(sn_opts, SNAPSHOT_OPT_ID),
>> +                                     qemu_opt_get(sn_opts, SNAPSHOT_OPT_NAME),
>> +&local_err);
>> +    } else if (sn_id_or_name) {
>> +        ret = bdrv_snapshot_load_tmp_by_id_or_name(bs, sn_id_or_name,
>> +&local_err);
>> +    }
>> +    if (ret<  0) {
>> +        errno = -ret;
>> +        err(EXIT_FAILURE,
>> +            "Failed to load snapshot: %s",
>> +            error_get_pretty(local_err));
>> +    }
>> +
>>       fd_size = bdrv_getlength(bs);
>>
>>       if (partition != -1) {
>> @@ -641,6 +670,10 @@ int main(int argc, char **argv)
>>           unlink(sockpath);
>>       }
>>
>> +    if (sn_opts) {
>> +        qemu_opts_del(sn_opts);
>> +    }
>> +
>>       if (device) {
>>           void *ret;
>>           pthread_join(client_thread,&ret);
>>
>

Re: [Qemu-devel] [PATCH V3 2/7] qemu-nbd: support internal snapshot export

[Wenchao Xia]

于 2013/10/10 14:00, Wenchao Xia 写道:
> 于 2013/10/2 0:08, Paolo Bonzini 写道:
>> Il 26/09/2013 02:16, Wenchao Xia ha scritto:
>>> Now it is possible to directly export an internal snapshot, which
>>> can be used to probe the snapshot's contents without qemu-img
>>> convert.
>>>
>>> Signed-off-by: Wenchao Xia<xiawenc@linux.vnet.ibm.com>
>>> ---
>>>   block/snapshot.c         |   18 ++++++++++++++++++
>>>   include/block/snapshot.h |    6 ++++++
>>>   qemu-nbd.c               |   35 ++++++++++++++++++++++++++++++++++-
>>>   3 files changed, 58 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/block/snapshot.c b/block/snapshot.c
>>> index 2ae3099..b371c27 100644
>>> --- a/block/snapshot.c
>>> +++ b/block/snapshot.c
>>> @@ -25,6 +25,24 @@
>>>   #include "block/snapshot.h"
>>>   #include "block/block_int.h"
>>>
>>> +QemuOptsList internal_snapshot_opts = {
>>> +    .name = "snapshot",
>>> +    .head = QTAILQ_HEAD_INITIALIZER(internal_snapshot_opts.head),
>>> +    .desc = {
>>> +        {
>>> +            .name = SNAPSHOT_OPT_ID,
>> Why not just use "id" and "name"?
>>
> Later it is used by code:
> qemu_opt_get(sn_opts, SNAPSHOT_OPT_ID),
> The macro is used to avoid type it twice in the codes, shouldn't it be 
> used?
>
> Another reason not using "id" is because string "id" is treated as 
> special case
> in opts_parse() so I choosed string "snapshot.id".
>
>>> +            .type = QEMU_OPT_STRING,
>>> +            .help = "snapshot id"
>>> +        },{
>>> +            .name = SNAPSHOT_OPT_NAME,
>>> +            .type = QEMU_OPT_STRING,
>>> +            .help = "snapshot name"
>>> +        },{
>>> +            /* end of list */
>>> +        }
>>> +    },
>>> +};
>>> +
>>>   int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo 
>>> *sn_info,
>>>                          const char *name)
>>>   {
>>> diff --git a/include/block/snapshot.h b/include/block/snapshot.h
>>> index d05bea7..c524a49 100644
>>> --- a/include/block/snapshot.h
>>> +++ b/include/block/snapshot.h
>>> @@ -27,6 +27,12 @@
>>>
>>>   #include "qemu-common.h"
>>>   #include "qapi/error.h"
>>> +#include "qemu/option.h"
>>> +
>>> +#define SNAPSHOT_OPT_ID         "snapshot.id"
>>> +#define SNAPSHOT_OPT_NAME       "snapshot.name"
>>> +
>>> +extern QemuOptsList internal_snapshot_opts;
>>>
>>>   typedef struct QEMUSnapshotInfo {
>>>       char id_str[128]; /* unique snapshot id */
>>> diff --git a/qemu-nbd.c b/qemu-nbd.c
>>> index c26c98e..6588a1f 100644
>>> --- a/qemu-nbd.c
>>> +++ b/qemu-nbd.c
>>> @@ -20,6 +20,7 @@
>>>   #include "block/block.h"
>>>   #include "block/nbd.h"
>>>   #include "qemu/main-loop.h"
>>> +#include "block/snapshot.h"
>>>
>>>   #include<stdarg.h>
>>>   #include<stdio.h>
>>> @@ -315,7 +316,9 @@ int main(int argc, char **argv)
>>>       char *device = NULL;
>>>       int port = NBD_DEFAULT_PORT;
>>>       off_t fd_size;
>>> -    const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:t";
>>> +    QemuOpts *sn_opts = NULL;
>>> +    const char *sn_id_or_name = NULL;
>>> +    const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:tl:L:";
>>>       struct option lopt[] = {
>>>           { "help", 0, NULL, 'h' },
>>>           { "version", 0, NULL, 'V' },
>>> @@ -328,6 +331,8 @@ int main(int argc, char **argv)
>>>           { "connect", 1, NULL, 'c' },
>>>           { "disconnect", 0, NULL, 'd' },
>>>           { "snapshot", 0, NULL, 's' },
>>> +        { "load-snapshot", 1, NULL, 'l' },
>> Just omit the long option here...
>>
>>> +        { "load-snapshot1", 1, NULL, 'L' },
>> ... and call this "load-snapshot".
>>
>> Paolo
>>
> OK, I will change as:
>
> { NULL, 1, NULL, 'l' },
>
> { "load-snapshot", 1, NULL, 'L' },
>
   From Eric's suggestion, I think simply one item:
{ "load-snapshot", 1, NULL, 'l' }
would be engough to handle both cases.

>>>           { "nocache", 0, NULL, 'n' },
>>>           { "cache", 1, NULL, QEMU_NBD_OPT_CACHE },
>>>   #ifdef CONFIG_LINUX_AIO
>>> @@ -428,6 +433,14 @@ int main(int argc, char **argv)
>>>                   errx(EXIT_FAILURE, "Offset must be positive `%s'", 
>>> optarg);
>>>               }
>>>               break;
>>> +        case 'l':
>>> +            sn_id_or_name = optarg;
>>> +            nbdflags |= NBD_FLAG_READ_ONLY;
>>> +            flags&= ~BDRV_O_RDWR;
>>> +            break;
>>> +        case 'L':
>>> +            sn_opts = qemu_opts_parse(&internal_snapshot_opts, 
>>> optarg, 0);
>>> +            /* fall through */
>>>           case 'r':
>>>               nbdflags |= NBD_FLAG_READ_ONLY;
>>>               flags&= ~BDRV_O_RDWR;
>>> @@ -581,6 +594,22 @@ int main(int argc, char **argv)
>>>               error_get_pretty(local_err));
>>>       }
>>>
>>> +    if (sn_opts) {
>>> +        ret = bdrv_snapshot_load_tmp(bs,
>>> +                                     qemu_opt_get(sn_opts, 
>>> SNAPSHOT_OPT_ID),
>>> +                                     qemu_opt_get(sn_opts, 
>>> SNAPSHOT_OPT_NAME),
>>> +&local_err);
>>> +    } else if (sn_id_or_name) {
>>> +        ret = bdrv_snapshot_load_tmp_by_id_or_name(bs, sn_id_or_name,
>>> +&local_err);
>>> +    }
>>> +    if (ret<  0) {
>>> +        errno = -ret;
>>> +        err(EXIT_FAILURE,
>>> +            "Failed to load snapshot: %s",
>>> +            error_get_pretty(local_err));
>>> +    }
>>> +
>>>       fd_size = bdrv_getlength(bs);
>>>
>>>       if (partition != -1) {
>>> @@ -641,6 +670,10 @@ int main(int argc, char **argv)
>>>           unlink(sockpath);
>>>       }
>>>
>>> +    if (sn_opts) {
>>> +        qemu_opts_del(sn_opts);
>>> +    }
>>> +
>>>       if (device) {
>>>           void *ret;
>>>           pthread_join(client_thread,&ret);
>>>
>>
>
>

[Qemu-devel] [PATCH V3 3/7] qemu-nbd: add doc for internal snapshot export

[Wenchao Xia]

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 qemu-nbd.c    |   11 ++++++++++-
 qemu-nbd.texi |   11 ++++++++++-
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index 6588a1f..49dfc14 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -80,7 +80,16 @@ static void usage(const char *name)
 "\n"
 "Block device options:\n"
 "  -r, --read-only      export read-only\n"
-"  -s, --snapshot       use snapshot file\n"
+"  -s, --snapshot       use FILE as an external snapshot, create a temporary\n"
+"                       file with backing_file=FILE, redirect the write to\n"
+"                       the temporary one\n"
+"  -l, --load-snapshot=SNAPSHOT_ID_OR_NAME\n"
+"                       load an internal snapshot inside FILE and export it\n"
+"                       as an read-only device\n"
+"  -L, --load-snapshot1=SNAPSHOT_PARAM\n"
+"                       load an internal snapshot inside FILE and export it\n"
+"                       as an read-only device, SNAPSHOT_PARAM format is\n"
+"                       'snapshot.id=[ID],snapshot.name=[NAME]'\n"
 "  -n, --nocache        disable host cache\n"
 "      --cache=MODE     set cache mode (none, writeback, ...)\n"
 #ifdef CONFIG_LINUX_AIO
diff --git a/qemu-nbd.texi b/qemu-nbd.texi
index 6055ec6..5940905 100644
--- a/qemu-nbd.texi
+++ b/qemu-nbd.texi
@@ -27,7 +27,16 @@ Export QEMU disk image using NBD protocol.
 @item -P, --partition=@var{num}
   only expose partition @var{num}
 @item -s, --snapshot
-  use snapshot file
+  use @var{filename} as an external snapshot, create a temporary
+  file with backing_file=@var{filename}, redirect the write to
+  the temporary one
+@item -l, --load-snapshot=@var{snapshot_id_or_name}
+  load an internal snapshot inside @var{filename} and export it
+  as an read-only device
+@item -L, --load-snapshot1=@var{snapshot_param}
+  load an internal snapshot inside @var{filename} and export it
+  as an read-only device, @var{snapshot_param} format is
+  'snapshot.id=[ID],snapshot.name=[NAME]'
 @item -n, --nocache
 @itemx --cache=@var{cache}
   set cache mode to be used with the file.  See the documentation of
-- 
1.7.1

Re: [Qemu-devel] [PATCH V3 3/7] qemu-nbd: add doc for internal snapshot export

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1503 bytes --]

On 09/25/2013 06:16 PM, Wenchao Xia wrote:
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  qemu-nbd.c    |   11 ++++++++++-
>  qemu-nbd.texi |   11 ++++++++++-
>  2 files changed, 20 insertions(+), 2 deletions(-)

This should be squashed into 2/7.  When adding new options, the
documentation should be added at the same time.

> +"                       the temporary one\n"
> +"  -l, --load-snapshot=SNAPSHOT_ID_OR_NAME\n"
> +"                       load an internal snapshot inside FILE and export it\n"
> +"                       as an read-only device\n"
> +"  -L, --load-snapshot1=SNAPSHOT_PARAM\n"
> +"                       load an internal snapshot inside FILE and export it\n"
> +"                       as an read-only device, SNAPSHOT_PARAM format is\n"
> +"                       'snapshot.id=[ID],snapshot.name=[NAME]'\n"

Why can't ONE option be good enough?  In other words, make the command
line parser smart enough so that:

--load-snapshot=name

tries SNAPSHOT_ID_OR_NAME, while

--load-snapshot=snapshot.id=xyz,snapshot.name=name

tries the SNAPSHOT_PARAM form.  In other words, if the optarg begins
with 'snapshot.', assume the SNAPSHOT_PARAM form, otherwise use the
SNAPSHOT_ID_OR_NAME form.  Then you only burn one short option letter,
and avoid the problem with ambiguous abbreviation that I complained
about in 2/7.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH V3 3/7] qemu-nbd: add doc for internal snapshot export

[Wenchao Xia]

于 2013/10/1 22:49, Eric Blake 写道:
> On 09/25/2013 06:16 PM, Wenchao Xia wrote:
>> Signed-off-by: Wenchao Xia<xiawenc@linux.vnet.ibm.com>
>> ---
>>   qemu-nbd.c    |   11 ++++++++++-
>>   qemu-nbd.texi |   11 ++++++++++-
>>   2 files changed, 20 insertions(+), 2 deletions(-)
> This should be squashed into 2/7.  When adding new options, the
> documentation should be added at the same time.
>
   OK.

>> +"                       the temporary one\n"
>> +"  -l, --load-snapshot=SNAPSHOT_ID_OR_NAME\n"
>> +"                       load an internal snapshot inside FILE and export it\n"
>> +"                       as an read-only device\n"
>> +"  -L, --load-snapshot1=SNAPSHOT_PARAM\n"
>> +"                       load an internal snapshot inside FILE and export it\n"
>> +"                       as an read-only device, SNAPSHOT_PARAM format is\n"
>> +"                       'snapshot.id=[ID],snapshot.name=[NAME]'\n"
> Why can't ONE option be good enough?  In other words, make the command
> line parser smart enough so that:
>
> --load-snapshot=name
>
> tries SNAPSHOT_ID_OR_NAME, while
>
> --load-snapshot=snapshot.id=xyz,snapshot.name=name
>
> tries the SNAPSHOT_PARAM form.  In other words, if the optarg begins
> with 'snapshot.', assume the SNAPSHOT_PARAM form, otherwise use the
> SNAPSHOT_ID_OR_NAME form.  Then you only burn one short option letter,
> and avoid the problem with ambiguous abbreviation that I complained
> about in 2/7.
>
   I split the option as two item since want to keep capatiability for
"-s snapshot.id=xyz" in qemu-img convert, it is possible some one already
named a snapshot as "snapshot.id=xyz". But from the comments of Paolo, I 
think
add  a new option in qemu-img convert and deprecate -s, can solve the 
problem,
so I will use your format in next version, thanks for tipping that.

[Qemu-devel] [PATCH V3 4/7] qemu-iotests: add 058 internal snapshot export with qemu-nbd case

[Wenchao Xia]

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 tests/qemu-iotests/058     |  104 ++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/058.out |   32 +++++++++++++
 tests/qemu-iotests/check   |    1 +
 tests/qemu-iotests/group   |    1 +
 4 files changed, 138 insertions(+), 0 deletions(-)
 create mode 100755 tests/qemu-iotests/058
 create mode 100644 tests/qemu-iotests/058.out

diff --git a/tests/qemu-iotests/058 b/tests/qemu-iotests/058
new file mode 100755
index 0000000..a1e8d62
--- /dev/null
+++ b/tests/qemu-iotests/058
@@ -0,0 +1,104 @@
+#!/bin/bash
+#
+# Test export internal snapshot by qemu-nbd.
+#
+# Copyright (C) 2013 IBM, Inc.
+#
+# Based on 029.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=xiawenc@linux.vnet.ibm.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+
+nbd_snapshot_port=10850
+nbd_snapshot_img="nbd:127.0.0.1:$nbd_snapshot_port"
+
+_export_nbd_snapshot()
+{
+    eval "$QEMU_NBD -v -t -b 127.0.0.1 -p $nbd_snapshot_port $TEST_IMG -l $1 &"
+    NBD_SNAPSHOT_PID=$!
+    sleep 1
+}
+
+_export_nbd_snapshot1()
+{
+    eval "$QEMU_NBD -v -t -b 127.0.0.1 -p $nbd_snapshot_port $TEST_IMG -L snapshot.name=$1 &"
+    NBD_SNAPSHOT_PID=$!
+    sleep 1
+}
+
+_cleanup()
+{
+    if [ -n "$NBD_SNAPSHOT_PID" ]; then
+        kill $NBD_SNAPSHOT_PID
+    fi
+	_cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+. ./common.pattern
+
+# Any format supporting intenal snapshots
+_supported_fmt qcow2
+_supported_proto generic
+_supported_os Linux
+
+echo
+echo "== preparing image =="
+_make_test_img 64M
+$QEMU_IO -c 'write -P 0xa 0x1000 0x1000' $TEST_IMG | _filter_qemu_io
+$QEMU_IO -c 'write -P 0xb 0x2000 0x1000' $TEST_IMG | _filter_qemu_io
+$QEMU_IMG snapshot -c sn1 $TEST_IMG
+$QEMU_IO -c 'write -P 0xc 0x1000 0x1000' $TEST_IMG | _filter_qemu_io
+$QEMU_IO -c 'write -P 0xd 0x2000 0x1000' $TEST_IMG | _filter_qemu_io
+_check_test_img
+
+echo
+echo "== verifying the image file with patterns =="
+$QEMU_IO -c 'read -P 0xc 0x1000 0x1000' $TEST_IMG | _filter_qemu_io
+$QEMU_IO -c 'read -P 0xd 0x2000 0x1000' $TEST_IMG | _filter_qemu_io
+
+_export_nbd_snapshot sn1
+
+echo
+echo "== verifying the exported snapshot with patterns =="
+$QEMU_IO -c 'read -P 0xa 0x1000 0x1000' $nbd_snapshot_img | _filter_qemu_io
+$QEMU_IO -c 'read -P 0xb 0x2000 0x1000' $nbd_snapshot_img | _filter_qemu_io
+
+kill $NBD_SNAPSHOT_PID
+sleep 1
+
+_export_nbd_snapshot1 sn1
+
+echo
+echo "== verifying the exported snapshot with patterns =="
+$QEMU_IO -c 'read -P 0xa 0x1000 0x1000' $nbd_snapshot_img | _filter_qemu_io
+$QEMU_IO -c 'read -P 0xb 0x2000 0x1000' $nbd_snapshot_img | _filter_qemu_io
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/058.out b/tests/qemu-iotests/058.out
new file mode 100644
index 0000000..cc4b8ca
--- /dev/null
+++ b/tests/qemu-iotests/058.out
@@ -0,0 +1,32 @@
+QA output created by 058
+
+== preparing image ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+wrote 4096/4096 bytes at offset 4096
+4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 4096/4096 bytes at offset 8192
+4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 4096/4096 bytes at offset 4096
+4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 4096/4096 bytes at offset 8192
+4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+No errors were found on the image.
+
+== verifying the image file with patterns ==
+read 4096/4096 bytes at offset 4096
+4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 4096/4096 bytes at offset 8192
+4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== verifying the exported snapshot with patterns ==
+read 4096/4096 bytes at offset 4096
+4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 4096/4096 bytes at offset 8192
+4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== verifying the exported snapshot with patterns ==
+read 4096/4096 bytes at offset 4096
+4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 4096/4096 bytes at offset 8192
+4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+*** done
diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index f5f328f..f879474 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -161,6 +161,7 @@ cat <<EOF
 QEMU          -- $QEMU
 QEMU_IMG      -- $QEMU_IMG
 QEMU_IO       -- $QEMU_IO
+QEMU_NBD      -- $QEMU_NBD
 IMGFMT        -- $FULL_IMGFMT_DETAILS
 IMGPROTO      -- $FULL_IMGPROTO_DETAILS
 PLATFORM      -- $FULL_HOST_DETAILS
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 1ad02e5..2793e1d 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -64,6 +64,7 @@
 055 rw auto
 056 rw auto backing
 057 rw auto
+058 rw auto
 059 rw auto
 060 rw auto
 061 rw auto
-- 
1.7.1

Re: [Qemu-devel] [PATCH V3 4/7] qemu-iotests: add 058 internal snapshot export with qemu-nbd case

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1493 bytes --]

On 09/25/2013 06:16 PM, Wenchao Xia wrote:
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---

> +_export_nbd_snapshot()
> +{
> +    eval "$QEMU_NBD -v -t -b 127.0.0.1 -p $nbd_snapshot_port $TEST_IMG -l $1 &"

Uggh.  Why do you need an eval here?  Especially given that there was
recently a patch to properly quote $TEST_IMG in case the tests are run
inside a directory whose absolute name included a space.  What's wrong
with just directly:

$QEMU_NBD -v -t -b 127.0.0.1 -p $nbd_snapshot_port "$TEST_IMG" -l $1 $

> +    NBD_SNAPSHOT_PID=$!
> +    sleep 1
> +}
> +
> +_export_nbd_snapshot1()
> +{
> +    eval "$QEMU_NBD -v -t -b 127.0.0.1 -p $nbd_snapshot_port $TEST_IMG -L snapshot.name=$1 &"

Likewise; and given my complaint on 2-3/7, it would be nicer to support
this with only one option name spelling.

> +_cleanup()
> +{
> +    if [ -n "$NBD_SNAPSHOT_PID" ]; then
> +        kill $NBD_SNAPSHOT_PID
> +    fi
> +	_cleanup_test_img

Kill the TAB, fix the indentation.

> +}
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +# get standard environment, filters and checks
> +. ./common.rc
> +. ./common.filter
> +. ./common.pattern
> +
> +# Any format supporting intenal snapshots

s/intenal/internal/

> +_supported_fmt qcow2
> +_supported_proto generic
> +_supported_os Linux

Is this test truly Linux-only?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH V3 4/7] qemu-iotests: add 058 internal snapshot export with qemu-nbd case

[Wenchao Xia]

于 2013/10/1 22:53, Eric Blake 写道:
> On 09/25/2013 06:16 PM, Wenchao Xia wrote:
>> Signed-off-by: Wenchao Xia<xiawenc@linux.vnet.ibm.com>
>> ---
>> +_export_nbd_snapshot()
>> +{
>> +    eval "$QEMU_NBD -v -t -b 127.0.0.1 -p $nbd_snapshot_port $TEST_IMG -l $1&"
> Uggh.  Why do you need an eval here?  Especially given that there was
> recently a patch to properly quote $TEST_IMG in case the tests are run
> inside a directory whose absolute name included a space.  What's wrong
> with just directly:
>
> $QEMU_NBD -v -t -b 127.0.0.1 -p $nbd_snapshot_port "$TEST_IMG" -l $1 $
>
   Just a copy and paste for "eval", will remove it.

>> +    NBD_SNAPSHOT_PID=$!
>> +    sleep 1
>> +}
>> +
>> +_export_nbd_snapshot1()
>> +{
>> +    eval "$QEMU_NBD -v -t -b 127.0.0.1 -p $nbd_snapshot_port $TEST_IMG -L snapshot.name=$1&"
> Likewise; and given my complaint on 2-3/7, it would be nicer to support
> this with only one option name spelling.
>
>> +_cleanup()
>> +{
>> +    if [ -n "$NBD_SNAPSHOT_PID" ]; then
>> +        kill $NBD_SNAPSHOT_PID
>> +    fi
>> +	_cleanup_test_img
> Kill the TAB, fix the indentation.
>
   Will fix.

>> +}
>> +trap "_cleanup; exit \$status" 0 1 2 3 15
>> +
>> +# get standard environment, filters and checks
>> +. ./common.rc
>> +. ./common.filter
>> +. ./common.pattern
>> +
>> +# Any format supporting intenal snapshots
> s/intenal/internal/
>
  will fix.

>> +_supported_fmt qcow2
>> +_supported_proto generic
>> +_supported_os Linux
> Is this test truly Linux-only?
   I think it is generic, will remove it.

[Qemu-devel] [PATCH V3 5/7] qemu-img: add -L for snapshot in convert

[Wenchao Xia]

Now qemu-img convert have similar options as qemu-nbd for internal
snapshot.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 qemu-img.c |   30 +++++++++++++++++++++---------
 1 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 6df58ed..a784ad4 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1140,6 +1140,7 @@ static int img_convert(int argc, char **argv)
     int min_sparse = 8; /* Need at least 4k of zeros for sparse detection */
     bool quiet = false;
     Error *local_err = NULL;
+    QemuOpts *sn_opts = NULL;
 
     fmt = NULL;
     out_fmt = "raw";
@@ -1148,7 +1149,7 @@ static int img_convert(int argc, char **argv)
     compress = 0;
     skip_create = 0;
     for(;;) {
-        c = getopt(argc, argv, "f:O:B:s:hce6o:pS:t:qn");
+        c = getopt(argc, argv, "f:O:B:s:hce6o:pS:t:qnL:");
         if (c == -1) {
             break;
         }
@@ -1183,6 +1184,9 @@ static int img_convert(int argc, char **argv)
         case 's':
             snapshot_name = optarg;
             break;
+        case 'L':
+            sn_opts = qemu_opts_parse(&internal_snapshot_opts, optarg, 0);
+            break;
         case 'S':
         {
             int64_t sval;
@@ -1254,7 +1258,12 @@ static int img_convert(int argc, char **argv)
         total_sectors += bs_sectors;
     }
 
-    if (snapshot_name != NULL) {
+    if (sn_opts) {
+        ret = bdrv_snapshot_load_tmp(bs[0],
+                                     qemu_opt_get(sn_opts, SNAPSHOT_OPT_ID),
+                                     qemu_opt_get(sn_opts, SNAPSHOT_OPT_NAME),
+                                     &local_err);
+    } else if (snapshot_name != NULL) {
         if (bs_n > 1) {
             error_report("No support for concatenating multiple snapshot");
             ret = -1;
@@ -1262,13 +1271,13 @@ static int img_convert(int argc, char **argv)
         }
 
         bdrv_snapshot_load_tmp_by_id_or_name(bs[0], snapshot_name, &local_err);
-        if (error_is_set(&local_err)) {
-            error_report("Failed to load snapshot: %s",
-                         error_get_pretty(local_err));
-            error_free(local_err);
-            ret = -1;
-            goto out;
-        }
+    }
+    if (error_is_set(&local_err)) {
+        error_report("Failed to load snapshot: %s",
+                     error_get_pretty(local_err));
+        error_free(local_err);
+        ret = -1;
+        goto out;
     }
 
     /* Find driver and parse its options */
@@ -1559,6 +1568,9 @@ out:
     free_option_parameters(create_options);
     free_option_parameters(param);
     qemu_vfree(buf);
+    if (sn_opts) {
+        qemu_opts_del(sn_opts);
+    }
     if (out_bs) {
         bdrv_unref(out_bs);
     }
-- 
1.7.1

Re: [Qemu-devel] [PATCH V3 5/7] qemu-img: add -L for snapshot in convert

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1165 bytes --]

On 09/25/2013 06:16 PM, Wenchao Xia wrote:
> Now qemu-img convert have similar options as qemu-nbd for internal
> snapshot.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  qemu-img.c |   30 +++++++++++++++++++++---------
>  1 files changed, 21 insertions(+), 9 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 6df58ed..a784ad4 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1140,6 +1140,7 @@ static int img_convert(int argc, char **argv)
>      int min_sparse = 8; /* Need at least 4k of zeros for sparse detection */
>      bool quiet = false;
>      Error *local_err = NULL;
> +    QemuOpts *sn_opts = NULL;
>  
>      fmt = NULL;
>      out_fmt = "raw";
> @@ -1148,7 +1149,7 @@ static int img_convert(int argc, char **argv)
>      compress = 0;
>      skip_create = 0;
>      for(;;) {
> -        c = getopt(argc, argv, "f:O:B:s:hce6o:pS:t:qn");
> +        c = getopt(argc, argv, "f:O:B:s:hce6o:pS:t:qnL:");

New options should be documented in the same patch that introduces them.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH V3 5/7] qemu-img: add -L for snapshot in convert

[Paolo Bonzini]

Il 26/09/2013 02:16, Wenchao Xia ha scritto:
> +        c = getopt(argc, argv, "f:O:B:s:hce6o:pS:t:qnL:");
>          if (c == -1) {
>              break;
>          }
> @@ -1183,6 +1184,9 @@ static int img_convert(int argc, char **argv)
>          case 's':
>              snapshot_name = optarg;
>              break;
> +        case 'L':
> +            sn_opts = qemu_opts_parse(&internal_snapshot_opts, optarg, 0);
> +            break;
>          case 'S':

Should qemu-img introduce -l too, and deprecate -s (continue to accept
it silently, but not document it)?

Paolo

Re: [Qemu-devel] [PATCH V3 5/7] qemu-img: add -L for snapshot in convert

[Wenchao Xia]

于 2013/10/2 0:07, Paolo Bonzini 写道:
> Il 26/09/2013 02:16, Wenchao Xia ha scritto:
>> +        c = getopt(argc, argv, "f:O:B:s:hce6o:pS:t:qnL:");
>>           if (c == -1) {
>>               break;
>>           }
>> @@ -1183,6 +1184,9 @@ static int img_convert(int argc, char **argv)
>>           case 's':
>>               snapshot_name = optarg;
>>               break;
>> +        case 'L':
>> +            sn_opts = qemu_opts_parse(&internal_snapshot_opts, optarg, 0);
>> +            break;
>>           case 'S':
> Should qemu-img introduce -l too, and deprecate -s (continue to accept
> it silently, but not document it)?
>
> Paolo
>
OK, will document both but mark it deprecated.

[Qemu-devel] [PATCH V3 6/7] qemu-img: add doc for param -L in convert

[Wenchao Xia]

Also renamed snapshot_name to snapshot_id_or_name to tip better.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 qemu-img-cmds.hx |    2 +-
 qemu-img.c       |    2 ++
 qemu-img.texi    |    7 +++++--
 3 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index da1d965..39f150c 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -34,7 +34,7 @@ STEXI
 ETEXI
 
 DEF("convert", img_convert,
-    "convert [-c] [-p] [-q] [-n] [-f fmt] [-t cache] [-O output_fmt] [-o options] [-s snapshot_name] [-S sparse_size] filename [filename2 [...]] output_filename")
+    "convert [-c] [-p] [-q] [-n] [-f fmt] [-t cache] [-O output_fmt] [-o options] [-s snapshot_id_or_name] [-L snapshot_param] [-S sparse_size] filename [filename2 [...]] output_filename")
 STEXI
 @item convert [-c] [-p] [-q] [-n] [-f @var{fmt}] [-t @var{cache}] [-O @var{output_fmt}] [-o @var{options}] [-s @var{snapshot_name}] [-S @var{sparse_size}] @var{filename} [@var{filename2} [...]] @var{output_filename}
 ETEXI
diff --git a/qemu-img.c b/qemu-img.c
index a784ad4..29eabaf 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -93,6 +93,8 @@ static void help(void)
            "  'options' is a comma separated list of format specific options in a\n"
            "    name=value format. Use -o ? for an overview of the options supported by the\n"
            "    used format\n"
+           "  'snapshot_param' is param used for internal snapshot, format is\n"
+           "    'snapshot.id=[ID],snapshot.name=[NAME]'\n"
            "  '-c' indicates that target image must be compressed (qcow format only)\n"
            "  '-u' enables unsafe rebasing. It is assumed that old and new backing file\n"
            "       match exactly. The image doesn't need a working backing file before\n"
diff --git a/qemu-img.texi b/qemu-img.texi
index 768054e..c025ba6 100644
--- a/qemu-img.texi
+++ b/qemu-img.texi
@@ -46,6 +46,9 @@ is the destination disk image filename
 is a comma separated list of format specific options in a
 name=value format. Use @code{-o ?} for an overview of the options supported
 by the used format or see the format descriptions below for details.
+@item snapshot_param
+is param used for internal snapshot, format is
+'snapshot.id=[ID],snapshot.name=[NAME]'
 
 
 @item -c
@@ -179,9 +182,9 @@ Error on reading data
 
 @end table
 
-@item convert [-c] [-p] [-n] [-f @var{fmt}] [-t @var{cache}] [-O @var{output_fmt}] [-o @var{options}] [-s @var{snapshot_name}] [-S @var{sparse_size}] @var{filename} [@var{filename2} [...]] @var{output_filename}
+@item convert [-c] [-p] [-n] [-f @var{fmt}] [-t @var{cache}] [-O @var{output_fmt}] [-o @var{options}] [-s @var{snapshot_id_or_name}] [-L @var{snapshot_param}] [-S @var{sparse_size}] @var{filename} [@var{filename2} [...]] @var{output_filename}
 
-Convert the disk image @var{filename} or a snapshot @var{snapshot_name} to disk image @var{output_filename}
+Convert the disk image @var{filename} or a snapshot @var{snapshot_id_or_name} to disk image @var{output_filename}
 using format @var{output_fmt}. It can be optionally compressed (@code{-c}
 option) or use any format specific options like encryption (@code{-o} option).
 
-- 
1.7.1

Re: [Qemu-devel] [PATCH V3 6/7] qemu-img: add doc for param -L in convert

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 869 bytes --]

On 09/25/2013 06:16 PM, Wenchao Xia wrote:
> Also renamed snapshot_name to snapshot_id_or_name to tip better.

s/to tip better/as a better hint of what it does/

> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  qemu-img-cmds.hx |    2 +-
>  qemu-img.c       |    2 ++
>  qemu-img.texi    |    7 +++++--
>  3 files changed, 8 insertions(+), 3 deletions(-)

Squash this into 5/7.

> +           "  'snapshot_param' is param used for internal snapshot, format is\n"
> +           "    'snapshot.id=[ID],snapshot.name=[NAME]'\n"

Again, can you reuse the existing -s, instead of having to add -L, by
making the command line parser smarter about whether it is seeing a
single name vs. a string starting with 'snapshot.'?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH V3 6/7] qemu-img: add doc for param -L in convert

[Wenchao Xia]

于 2013/10/1 22:56, Eric Blake 写道:
> On 09/25/2013 06:16 PM, Wenchao Xia wrote:
>> Also renamed snapshot_name to snapshot_id_or_name to tip better.
> s/to tip better/as a better hint of what it does/
>
>> Signed-off-by: Wenchao Xia<xiawenc@linux.vnet.ibm.com>
>> ---
>>   qemu-img-cmds.hx |    2 +-
>>   qemu-img.c       |    2 ++
>>   qemu-img.texi    |    7 +++++--
>>   3 files changed, 8 insertions(+), 3 deletions(-)
> Squash this into 5/7.
>
   OK.

>> +           "  'snapshot_param' is param used for internal snapshot, format is\n"
>> +           "    'snapshot.id=[ID],snapshot.name=[NAME]'\n"
> Again, can you reuse the existing -s, instead of having to add -L, by
   There may be compatiability issue for existing user, I think add -l 
and deprecate old -s,
would be better.

> making the command line parser smarter about whether it is seeing a
> single name vs. a string starting with 'snapshot.'?
>

[Qemu-devel] [PATCH V3 7/7] qemu-iotests: add test for snapshot in qemu-img convert

[Wenchao Xia]

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 tests/qemu-iotests/058     |   19 ++++++++++++++++++-
 tests/qemu-iotests/058.out |   12 ++++++++++++
 2 files changed, 30 insertions(+), 1 deletions(-)

diff --git a/tests/qemu-iotests/058 b/tests/qemu-iotests/058
index a1e8d62..ec7e06d 100755
--- a/tests/qemu-iotests/058
+++ b/tests/qemu-iotests/058
@@ -1,6 +1,6 @@
 #!/bin/bash
 #
-# Test export internal snapshot by qemu-nbd.
+# Test export internal snapshot by qemu-nbd, convert it by qemu-img.
 #
 # Copyright (C) 2013 IBM, Inc.
 #
@@ -33,6 +33,8 @@ status=1	# failure is the default!
 nbd_snapshot_port=10850
 nbd_snapshot_img="nbd:127.0.0.1:$nbd_snapshot_port"
 
+converted_image=$TEST_IMG.converted
+
 _export_nbd_snapshot()
 {
     eval "$QEMU_NBD -v -t -b 127.0.0.1 -p $nbd_snapshot_port $TEST_IMG -l $1 &"
@@ -53,6 +55,7 @@ _cleanup()
         kill $NBD_SNAPSHOT_PID
     fi
 	_cleanup_test_img
+        rm -f $converted_image
 }
 trap "_cleanup; exit \$status" 0 1 2 3 15
 
@@ -98,6 +101,20 @@ echo "== verifying the exported snapshot with patterns =="
 $QEMU_IO -c 'read -P 0xa 0x1000 0x1000' $nbd_snapshot_img | _filter_qemu_io
 $QEMU_IO -c 'read -P 0xb 0x2000 0x1000' $nbd_snapshot_img | _filter_qemu_io
 
+$QEMU_IMG convert $TEST_IMG -s sn1 -O qcow2 $converted_image
+
+echo
+echo "== verifying the converted snapshot with patterns =="
+$QEMU_IO -c 'read -P 0xa 0x1000 0x1000' $converted_image | _filter_qemu_io
+$QEMU_IO -c 'read -P 0xb 0x2000 0x1000' $converted_image | _filter_qemu_io
+
+$QEMU_IMG convert $TEST_IMG -L snapshot.name=sn1 -O qcow2 $converted_image
+
+echo
+echo "== verifying the converted snapshot with patterns =="
+$QEMU_IO -c 'read -P 0xa 0x1000 0x1000' $converted_image | _filter_qemu_io
+$QEMU_IO -c 'read -P 0xb 0x2000 0x1000' $converted_image | _filter_qemu_io
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/058.out b/tests/qemu-iotests/058.out
index cc4b8ca..a8381b9 100644
--- a/tests/qemu-iotests/058.out
+++ b/tests/qemu-iotests/058.out
@@ -29,4 +29,16 @@ read 4096/4096 bytes at offset 4096
 4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 read 4096/4096 bytes at offset 8192
 4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== verifying the converted snapshot with patterns ==
+read 4096/4096 bytes at offset 4096
+4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 4096/4096 bytes at offset 8192
+4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== verifying the converted snapshot with patterns ==
+read 4096/4096 bytes at offset 4096
+4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 4096/4096 bytes at offset 8192
+4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 *** done
-- 
1.7.1

Re: [Qemu-devel] [PATCH V3 7/7] qemu-iotests: add test for snapshot in qemu-img convert

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 382 bytes --]

On 09/25/2013 06:16 PM, Wenchao Xia wrote:
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---

> @@ -53,6 +55,7 @@ _cleanup()
>          kill $NBD_SNAPSHOT_PID
>      fi
>  	_cleanup_test_img
> +        rm -f $converted_image

Indentation is off.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH V3 7/7] qemu-iotests: add test for snapshot in qemu-img convert

[Wenchao Xia]

于 2013/10/1 22:57, Eric Blake 写道:
> On 09/25/2013 06:16 PM, Wenchao Xia wrote:
>> Signed-off-by: Wenchao Xia<xiawenc@linux.vnet.ibm.com>
>> ---
>> @@ -53,6 +55,7 @@ _cleanup()
>>           kill $NBD_SNAPSHOT_PID
>>       fi
>>   	_cleanup_test_img
>> +        rm -f $converted_image
> Indentation is off.
>
  will fix.

Re: [Qemu-devel] [PATCH V3 0/7] export internal snapshot by qemu-nbd

[Wenchao Xia]

于 2013/9/26 8:16, Wenchao Xia 写道:
> This series allow user to read internal snapshot's contents without qemu-img
> convert.
>
> V2:
>   Address Stefan's comments:
>   02: add 'fall through' comments in the case statement.
>   03: add doc about the difference of internal snapshot and backing chain
> snapshot, which is used in previous '--snapshot' parameter.
>   Other:
>   01,04: rebased on upstream with conflict resolved.
>
> v3:
>   Address Paolo's comments:
>   02: add parameter "-l snapshot_id_or_name", rename options
> snapshot-load to load-snapshot, use QemuOpts.
>   03: rename snapshot-load to load-snapshot.
>   04: related change to test both -l and -L case.
>   05-07: add similar parameter for qemu-img convert.
>   Other:
>   01: foldered original snapshot logic into function
> bdrv_snapshot_load_tmp_by_id_or_name(), since multiple
> caller present in this version. Refined error message from
> ", reason: %s" to ": %s".
>   02: Refined error message from ", reason: %s" to ": %s".
>   03: Rename PARAM to SNAPSHOT_PARAM.
>
> Wenchao Xia (7):
>   1 snapshot: distinguish id and name in load_tmp
>   2 qemu-nbd: support internal snapshot export
>   3 qemu-nbd: add doc for internal snapshot export
>   4 qemu-iotests: add 058 internal snapshot export with qemu-nbd case
>   5 qemu-img: add -L for snapshot in convert
>   6 qemu-img: add doc for param -L in convert
>   7 qemu-iotests: add test for snapshot in qemu-img convert
>
>  block/qcow2-snapshot.c     |   16 +++++-
>  block/qcow2.h              |    5 ++-
>  block/snapshot.c           |   76 +++++++++++++++++++++++++++-
>  include/block/block_int.h  |    4 +-
>  include/block/snapshot.h   |   13 ++++-
>  qemu-img-cmds.hx           |    2 +-
>  qemu-img.c                 |   32 +++++++++---
>  qemu-img.texi              |    7 ++-
>  qemu-nbd.c                 |   46 ++++++++++++++++-
>  qemu-nbd.texi              |   11 ++++-
>  tests/qemu-iotests/058     |  121 ++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/058.out |   44 ++++++++++++++++
>  tests/qemu-iotests/check   |    1 +
>  tests/qemu-iotests/group   |    1 +
>  14 files changed, 359 insertions(+), 20 deletions(-)
>  create mode 100755 tests/qemu-iotests/058
>  create mode 100644 tests/qemu-iotests/058.out
>
Any commenst for this simple series? :)