[Qemu-devel] [PATCH V7 0/6] export internal snapshot by qemu-nbd

[Qemu-devel] [PATCH V7 0/6] 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.

v4:
  Address Eric's comments:
  01: typo fix.
  02: squashed patch. Use only one option -l and distiguish
the cases by the starting string.
  03: remove 'eval', remove '_supported_os Linux', remove the
comments that have typo.
  04: squashed patch. Add only one option -l and distiguish
the cases by the starting string.
  05: remove indentation.
  Address Paolo's comments:
  02: Use only one option -l and distiguish the cases by
the starting string.
  04: Use only one option -l and distiguish the cases by
the starting string, mark old -s as deprecated, added doc
for them.

v5:
  Address Jeff's comments:
  Quote image name in test cases.

v6:
  Address Kevin's comments:
  1: typo fix, remove device and snapshot info in error message.
  2: use strstart().
  3: use _require_command(), limit proto to file, since when proto=nbd
it can't work. also changed _require_command() to tip better.
  4: use strstart().
  6: new patch, since I found the doc missing in debugging.

v7:
  Address Stefan's comments:
  3: use unix socket and wait for 30s, to make the case reliable, folder
  4: add missing doc change.
  Other:
  3,5: add label "method 1"/"method 2" in echo message, to tip better.

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

 block/qcow2-snapshot.c       |   10 +++-
 block/qcow2.h                |    5 +-
 block/snapshot.c             |   77 ++++++++++++++++++++++-
 include/block/block_int.h    |    4 +-
 include/block/snapshot.h     |   15 ++++-
 qemu-img-cmds.hx             |    4 +-
 qemu-img.c                   |   44 +++++++++++--
 qemu-img.texi                |   12 +++-
 qemu-nbd.c                   |   47 ++++++++++++++-
 qemu-nbd.texi                |   10 +++-
 tests/qemu-iotests/058       |  138 ++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/058.out   |   44 +++++++++++++
 tests/qemu-iotests/check     |    1 +
 tests/qemu-iotests/common.rc |    3 +-
 tests/qemu-iotests/group     |    1 +
 15 files changed, 390 insertions(+), 25 deletions(-)
 create mode 100755 tests/qemu-iotests/058
 create mode 100644 tests/qemu-iotests/058.out

[Qemu-devel] [PATCH V7 1/6] 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.
Also fix a typo in comments of bdrv_snapshot_delete().

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

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 3529c68..ad8bf3d 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -675,7 +675,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;
@@ -687,8 +690,10 @@ int qcow2_snapshot_load_tmp(BlockDriverState *bs, const char *snapshot_name)
     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 snapshot");
         return -ENOENT;
     }
     sn = &s->snapshots[snapshot_index];
@@ -699,6 +704,7 @@ 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");
         g_free(new_l1_table);
         return ret;
     }
diff --git a/block/qcow2.h b/block/qcow2.h
index 922e190..303eb26 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -488,7 +488,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..565222e 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -194,7 +194,7 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
  * If only @snapshot_id is specified, delete the first one with id
  * @snapshot_id.
  * If only @name is specified, delete the first one with name @name.
- * if none is specified, return -ENINVAL.
+ * if none is specified, return -EINVAL.
  *
  * Returns: 0 on success, -errno on failure. If @bs is not inserted, return
  * -ENOMEDIUM. If @snapshot_id and @name are both NULL, return -EINVAL. If @bs
@@ -265,18 +265,71 @@ 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 -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 a matching @id and
+ * @name, return -ENOENT. 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;
+
     if (!drv) {
+        error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, bdrv_get_device_name(bs));
         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 is not readonly");
         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, bdrv_get_device_name(bs),
+              "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 1666066..8bbfb09 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -175,7 +175,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);
     ImageInfoSpecific *(*bdrv_get_specific_info)(BlockDriverState *bs);
 
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 b6b5644..a73beb5 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

[Qemu-devel] [PATCH V7 2/6] 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 |    8 ++++++++
 qemu-nbd.c               |   46 ++++++++++++++++++++++++++++++++++++++++++++--
 qemu-nbd.texi            |    8 +++++++-
 4 files changed, 77 insertions(+), 3 deletions(-)

diff --git a/block/snapshot.c b/block/snapshot.c
index 565222e..9047f8d 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..770d9bb 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -27,6 +27,14 @@
 
 #include "qemu-common.h"
 #include "qapi/error.h"
+#include "qemu/option.h"
+
+
+#define SNAPSHOT_OPT_BASE       "snapshot."
+#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..fe6053c 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>
@@ -79,7 +80,14 @@ 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_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]', or\n"
+"                       '[ID_OR_NAME]'\n"
 "  -n, --nocache        disable host cache\n"
 "      --cache=MODE     set cache mode (none, writeback, ...)\n"
 #ifdef CONFIG_LINUX_AIO
@@ -315,7 +323,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:";
     struct option lopt[] = {
         { "help", 0, NULL, 'h' },
         { "version", 0, NULL, 'V' },
@@ -328,6 +338,7 @@ int main(int argc, char **argv)
         { "connect", 1, NULL, 'c' },
         { "disconnect", 0, NULL, 'd' },
         { "snapshot", 0, NULL, 's' },
+        { "load-snapshot", 1, NULL, 'l' },
         { "nocache", 0, NULL, 'n' },
         { "cache", 1, NULL, QEMU_NBD_OPT_CACHE },
 #ifdef CONFIG_LINUX_AIO
@@ -428,6 +439,17 @@ int main(int argc, char **argv)
                 errx(EXIT_FAILURE, "Offset must be positive `%s'", optarg);
             }
             break;
+        case 'l':
+            if (strstart(optarg, SNAPSHOT_OPT_BASE, NULL)) {
+                sn_opts = qemu_opts_parse(&internal_snapshot_opts, optarg, 0);
+                if (!sn_opts) {
+                    errx(EXIT_FAILURE, "Failed in parsing snapshot param `%s'",
+                         optarg);
+                }
+            } else {
+                sn_id_or_name = optarg;
+            }
+            /* fall through */
         case 'r':
             nbdflags |= NBD_FLAG_READ_ONLY;
             flags &= ~BDRV_O_RDWR;
@@ -581,6 +603,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 +679,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);
diff --git a/qemu-nbd.texi b/qemu-nbd.texi
index 6055ec6..5b55f76 100644
--- a/qemu-nbd.texi
+++ b/qemu-nbd.texi
@@ -27,7 +27,13 @@ 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_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]' or '[ID_OR_NAME]'
 @item -n, --nocache
 @itemx --cache=@var{cache}
   set cache mode to be used with the file.  See the documentation of
-- 
1.7.1

[Qemu-devel] [PATCH V7 3/6] qemu-iotests: add 058 internal snapshot export with qemu-nbd case

[Wenchao Xia]

This case can't run when IMGPROTO=nbd, since it needs to create some
internal snapshot which would fail for EOF write request, even when
TEST_IMG is exported with "-f raw" in common.rc, so set _supported_proto
to file.

_require_command() is changed to tip what util is missing, instead
of printing a blank.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 tests/qemu-iotests/058       |  121 ++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/058.out   |   32 +++++++++++
 tests/qemu-iotests/check     |    1 +
 tests/qemu-iotests/common.rc |    3 +-
 tests/qemu-iotests/group     |    1 +
 5 files changed, 157 insertions(+), 1 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..cf50857
--- /dev/null
+++ b/tests/qemu-iotests/058
@@ -0,0 +1,121 @@
+#!/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_unix_socket=$TEST_DIR/test_qemu_nbd_socket
+nbd_snapshot_img="nbd:unix:$nbd_unix_socket"
+
+_cleanup_nbd()
+{
+    if [ -n "$NBD_SNAPSHOT_PID" ]; then
+        kill "$NBD_SNAPSHOT_PID"
+    fi
+    rm -f "$nbd_unix_socket"
+}
+
+_wait_for_nbd()
+{
+    for ((i = 0; i < 300; i++))
+    do
+        if [ -r "$nbd_unix_socket" ]; then
+            return
+        fi
+        sleep 0.1
+    done
+    echo "Failed in check of unix socket created by qemu-nbd"
+    exit 1
+}
+
+_export_nbd_snapshot()
+{
+    _cleanup_nbd
+    $QEMU_NBD -v -t -k "$nbd_unix_socket" "$TEST_IMG" -l $1 &
+    NBD_SNAPSHOT_PID=$!
+    _wait_for_nbd
+}
+
+_export_nbd_snapshot1()
+{
+    _cleanup_nbd
+    $QEMU_NBD -v -t -k "$nbd_unix_socket" "$TEST_IMG" -l snapshot.name=$1 &
+    NBD_SNAPSHOT_PID=$!
+    _wait_for_nbd
+}
+
+_cleanup()
+{
+    _cleanup_nbd
+    _cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+. ./common.pattern
+
+_supported_fmt qcow2
+_supported_proto file
+_require_command QEMU_NBD
+
+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, method 1 =="
+$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
+
+_export_nbd_snapshot1 sn1
+
+echo
+echo "== verifying the exported snapshot with patterns, method 2 =="
+$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..768ac61
--- /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, method 1 ==
+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, method 2 ==
+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/common.rc b/tests/qemu-iotests/common.rc
index 7f62457..9e9979a 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -405,7 +405,8 @@ _unsupported_qemu_io_options()
 #
 _require_command()
 {
-    [ -x "$1" ] || _notrun "$1 utility required, skipped this test"
+    eval c=\$$1
+    [ -x "$c" ] || _notrun "$1 utility required, skipped this test"
 }
 
 _full_imgfmt_details()
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index b63b18c..bd6acb9 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

[Qemu-devel] [PATCH V7 4/6] 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-cmds.hx |    4 ++--
 qemu-img.c       |   44 +++++++++++++++++++++++++++++++++++---------
 qemu-img.texi    |   12 ++++++++----
 3 files changed, 45 insertions(+), 15 deletions(-)

diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index da1d965..d029609 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -34,9 +34,9 @@ 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}
+@item convert [-c] [-p] [-q] [-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}
 ETEXI
 
 DEF("info", img_info,
diff --git a/qemu-img.c b/qemu-img.c
index a73beb5..ee940c2 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -93,6 +93,11 @@ 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\n"
+           "    is 'snapshot.id=[ID],snapshot.name=[NAME]', or\n"
+           "    '[ID_OR_NAME]'\n"
+           "  'snapshot_id_or_name' is deprecated, use 'snapshot_param'\n"
+           "    instead\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"
@@ -1140,6 +1145,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 +1154,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 +1189,18 @@ static int img_convert(int argc, char **argv)
         case 's':
             snapshot_name = optarg;
             break;
+        case 'l':
+            if (strstart(optarg, SNAPSHOT_OPT_BASE, NULL)) {
+                sn_opts = qemu_opts_parse(&internal_snapshot_opts, optarg, 0);
+                if (!sn_opts) {
+                    error_report("Failed in parsing snapshot param '%s'",
+                                 optarg);
+                    return 1;
+                }
+            } else {
+                snapshot_name = optarg;
+            }
+            break;
         case 'S':
         {
             int64_t sval;
@@ -1254,7 +1272,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 +1285,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 +1582,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);
     }
diff --git a/qemu-img.texi b/qemu-img.texi
index 768054e..fbdbfa7 100644
--- a/qemu-img.texi
+++ b/qemu-img.texi
@@ -46,7 +46,11 @@ 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]' or '[ID_OR_NAME]'
+@item snapshot_id_or_name
+is deprecated, use snapshot_param instead
 
 @item -c
 indicates that target image must be compressed (qcow format only)
@@ -179,10 +183,10 @@ 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}
-using format @var{output_fmt}. It can be optionally compressed (@code{-c}
+Convert the disk image @var{filename} or a snapshot @var{snapshot_param}(@var{snapshot_id_or_name} is deprecated)
+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).
 
 Only the formats @code{qcow} and @code{qcow2} support compression. The
-- 
1.7.1

[Qemu-devel] [PATCH V7 5/6] 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 cf50857..14584cd 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.
 #
@@ -54,6 +54,8 @@ _wait_for_nbd()
     exit 1
 }
 
+converted_image=$TEST_IMG.converted
+
 _export_nbd_snapshot()
 {
     _cleanup_nbd
@@ -74,6 +76,7 @@ _cleanup()
 {
     _cleanup_nbd
     _cleanup_test_img
+    rm -f "$converted_image"
 }
 trap "_cleanup; exit \$status" 0 1 2 3 15
 
@@ -115,6 +118,20 @@ echo "== verifying the exported snapshot with patterns, method 2 =="
 $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" -l sn1 -O qcow2 "$converted_image"
+
+echo
+echo "== verifying the converted snapshot with patterns, method 1 =="
+$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, method 2 =="
+$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 768ac61..9a69379 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, method 1 ==
+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, method 2 ==
+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

[Qemu-devel] [PATCH V7 6/6] qemu-nbd: add doc for option -f

[Wenchao Xia]

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

diff --git a/qemu-nbd.c b/qemu-nbd.c
index fe6053c..136e8c9 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -79,6 +79,7 @@ static void usage(const char *name)
 #endif
 "\n"
 "Block device options:\n"
+"  -f, --format=FORMAT  set image format (raw, qcow2, ...)\n"
 "  -r, --read-only      export read-only\n"
 "  -s, --snapshot       use FILE as an external snapshot, create a temporary\n"
 "                       file with backing_file=FILE, redirect the write to\n"
diff --git a/qemu-nbd.texi b/qemu-nbd.texi
index 5b55f76..0a7e013 100644
--- a/qemu-nbd.texi
+++ b/qemu-nbd.texi
@@ -22,6 +22,8 @@ Export QEMU disk image using NBD protocol.
   interface to bind to (default @samp{0.0.0.0})
 @item -k, --socket=@var{path}
   Use a unix socket with path @var{path}
+@item -f, --format=@var{format}
+  Set image format as @var{format}
 @item -r, --read-only
   export read-only
 @item -P, --partition=@var{num}
-- 
1.7.1

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

[Stefan Hajnoczi]

On Wed, Dec 04, 2013 at 05:10:53PM +0800, Wenchao Xia wrote:
> 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.
> 
> v4:
>   Address Eric's comments:
>   01: typo fix.
>   02: squashed patch. Use only one option -l and distiguish
> the cases by the starting string.
>   03: remove 'eval', remove '_supported_os Linux', remove the
> comments that have typo.
>   04: squashed patch. Add only one option -l and distiguish
> the cases by the starting string.
>   05: remove indentation.
>   Address Paolo's comments:
>   02: Use only one option -l and distiguish the cases by
> the starting string.
>   04: Use only one option -l and distiguish the cases by
> the starting string, mark old -s as deprecated, added doc
> for them.
> 
> v5:
>   Address Jeff's comments:
>   Quote image name in test cases.
> 
> v6:
>   Address Kevin's comments:
>   1: typo fix, remove device and snapshot info in error message.
>   2: use strstart().
>   3: use _require_command(), limit proto to file, since when proto=nbd
> it can't work. also changed _require_command() to tip better.
>   4: use strstart().
>   6: new patch, since I found the doc missing in debugging.
> 
> v7:
>   Address Stefan's comments:
>   3: use unix socket and wait for 30s, to make the case reliable, folder
>   4: add missing doc change.
>   Other:
>   3,5: add label "method 1"/"method 2" in echo message, to tip better.
> 
> Wenchao Xia (6):
>   1 snapshot: distinguish id and name in load_tmp
>   2 qemu-nbd: support internal snapshot export
>   3 qemu-iotests: add 058 internal snapshot export with qemu-nbd case
>   4 qemu-img: add -l for snapshot in convert
>   5 qemu-iotests: add test for snapshot in qemu-img convert
>   6 qemu-nbd: add doc for option -f
> 
>  block/qcow2-snapshot.c       |   10 +++-
>  block/qcow2.h                |    5 +-
>  block/snapshot.c             |   77 ++++++++++++++++++++++-
>  include/block/block_int.h    |    4 +-
>  include/block/snapshot.h     |   15 ++++-
>  qemu-img-cmds.hx             |    4 +-
>  qemu-img.c                   |   44 +++++++++++--
>  qemu-img.texi                |   12 +++-
>  qemu-nbd.c                   |   47 ++++++++++++++-
>  qemu-nbd.texi                |   10 +++-
>  tests/qemu-iotests/058       |  138 ++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/058.out   |   44 +++++++++++++
>  tests/qemu-iotests/check     |    1 +
>  tests/qemu-iotests/common.rc |    3 +-
>  tests/qemu-iotests/group     |    1 +
>  15 files changed, 390 insertions(+), 25 deletions(-)
>  create mode 100755 tests/qemu-iotests/058
>  create mode 100644 tests/qemu-iotests/058.out
> 
> 

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan

Re: [Qemu-devel] [PATCH V7 4/6] qemu-img: add -l for snapshot in convert

[Eric Blake]

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

On 12/04/2013 02:10 AM, 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>
> ---

> @@ -1183,6 +1189,18 @@ static int img_convert(int argc, char **argv)
>          case 's':
>              snapshot_name = optarg;
>              break;
> +        case 'l':
> +            if (strstart(optarg, SNAPSHOT_OPT_BASE, NULL)) {
> +                sn_opts = qemu_opts_parse(&internal_snapshot_opts, optarg, 0);
> +                if (!sn_opts) {
> +                    error_report("Failed in parsing snapshot param '%s'",
> +                                 optarg);
> +                    return 1;
> +                }
> +            } else {
> +                snapshot_name = optarg;
> +            }
> +            break;

Do we want a followup patch that makes it an error to use -l and -s
together?  Without such a patch, we have the odd behavior that:

convert -l name1 -s name2

loads name2, but:

convert -l snapshot.name=name1 -s name2

loads name1.  Confusing that the choice of HOW the argument to -l is
specified determines whether the -s has any impact.

For that matter, why can't '-s' and '-l' be made synonyms of each other?
 In other words, why not support:

convert -s snapshot.name=name1

-- 
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 V7 4/6] qemu-img: add -l for snapshot in convert

[Wenchao Xia]

于 2013/12/5 4:42, Eric Blake 写道:
> On 12/04/2013 02:10 AM, 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>
>> ---
>
>> @@ -1183,6 +1189,18 @@ static int img_convert(int argc, char **argv)
>>           case 's':
>>               snapshot_name = optarg;
>>               break;
>> +        case 'l':
>> +            if (strstart(optarg, SNAPSHOT_OPT_BASE, NULL)) {
>> +                sn_opts = qemu_opts_parse(&internal_snapshot_opts, optarg, 0);
>> +                if (!sn_opts) {
>> +                    error_report("Failed in parsing snapshot param '%s'",
>> +                                 optarg);
>> +                    return 1;
>> +                }
>> +            } else {
>> +                snapshot_name = optarg;
>> +            }
>> +            break;
>
> Do we want a followup patch that makes it an error to use -l and -s
> together?  Without such a patch, we have the odd behavior that:
>
> convert -l name1 -s name2
>
> loads name2, but:
>
> convert -l snapshot.name=name1 -s name2
>
> loads name1.  Confusing that the choice of HOW the argument to -l is
> specified determines whether the -s has any impact.
>
> For that matter, why can't '-s' and '-l' be made synonyms of each other?
>   In other words, why not support:
>
> convert -s snapshot.name=name1
>
   Previous I planned to use -l for internal snapshot in all possible
program, since -s is taken as external snapshot in qemu, qemu-nbd.
let -s stands for internal in qemu-img convert only, may bring
confuse to user, so I deprecated it instead of enhance it(I want
to remove it but may bring compatiablity issue).
   Yes, it should report error when both specified, will send a patch
if you agree '-l' should still be used.

Re: [Qemu-devel] [PATCH V7 4/6] qemu-img: add -l for snapshot in convert

[Wenchao Xia]

>>
>> convert -s snapshot.name=name1
>>
>    Previous I planned to use -l for internal snapshot in all possible
> program, since -s is taken as external snapshot in qemu, qemu-nbd.
> let -s stands for internal in qemu-img convert only, may bring
> confuse to user, so I deprecated it instead of enhance it(I want
> to remove it but may bring compatiablity issue).
>    Yes, it should report error when both specified, will send a patch
> if you agree '-l' should still be used.
>
>
   Eric, I hope to get your idea before patching, any comments?

Re: [Qemu-devel] [PATCH V7 4/6] qemu-img: add -l for snapshot in convert

[Eric Blake]

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

On 12/08/2013 08:43 PM, Wenchao Xia wrote:
>>>
>>> convert -s snapshot.name=name1
>>>
>>    Previous I planned to use -l for internal snapshot in all possible
>> program, since -s is taken as external snapshot in qemu, qemu-nbd.

Consistency in command line options between different tools is nice, but
is less important than adding functionality.  I'm perfectly fine if we
use -l in one tool and -s in another, as long as the documentation is
clear on how to spell the option for the tool I want to use.

>> let -s stands for internal in qemu-img convert only, may bring
>> confuse to user, so I deprecated it instead of enhance it(I want
>> to remove it but may bring compatiablity issue).
>>    Yes, it should report error when both specified, will send a patch
>> if you agree '-l' should still be used.
>>
>>
>   Eric, I hope to get your idea before patching, any comments?
> 

My biggest concern was that by adding -l as a superset of -s, but not
taking care of the relation between the two, you created odd command
line usage patterns.  For qemu-img, it may be simpler to just make -s do
everything, instead of trying to deprecate it (that is, adding -l for
consistency with other tools while breaking -s isn't nice).

-- 
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 V7 4/6] qemu-img: add -l for snapshot in convert

[Wenchao Xia]

于 2013/12/13 21:44, Eric Blake 写道:
> On 12/08/2013 08:43 PM, Wenchao Xia wrote:
>>>>
>>>> convert -s snapshot.name=name1
>>>>
>>>     Previous I planned to use -l for internal snapshot in all possible
>>> program, since -s is taken as external snapshot in qemu, qemu-nbd.
>
> Consistency in command line options between different tools is nice, but
> is less important than adding functionality.  I'm perfectly fine if we
> use -l in one tool and -s in another, as long as the documentation is
> clear on how to spell the option for the tool I want to use.
>
>>> let -s stands for internal in qemu-img convert only, may bring
>>> confuse to user, so I deprecated it instead of enhance it(I want
>>> to remove it but may bring compatiablity issue).
>>>     Yes, it should report error when both specified, will send a patch
>>> if you agree '-l' should still be used.
>>>
>>>
>>    Eric, I hope to get your idea before patching, any comments?
>>
>
> My biggest concern was that by adding -l as a superset of -s, but not
> taking care of the relation between the two, you created odd command
> line usage patterns.  For qemu-img, it may be simpler to just make -s do
> everything, instead of trying to deprecate it (that is, adding -l for
> consistency with other tools while breaking -s isn't nice).
>
   OK, there is still one cornor case to consider:
-s snapshot.name=name1
   It may change the semantics if a caller used qemu-img convert as
above before, although it seems insane.:)

Re: [Qemu-devel] [PATCH V7 3/6] qemu-iotests: add 058 internal snapshot export with qemu-nbd case

[Kevin Wolf]

Am 04.12.2013 um 10:10 hat Wenchao Xia geschrieben:
> This case can't run when IMGPROTO=nbd, since it needs to create some
> internal snapshot which would fail for EOF write request, even when
> TEST_IMG is exported with "-f raw" in common.rc, so set _supported_proto
> to file.
> 
> _require_command() is changed to tip what util is missing, instead
> of printing a blank.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>

> --- 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

Please, please, please, stop doing this.

If you see a hole in the numbering of test cases, it is because there
are yet unmerged patches that use this number. The only thing you
achieve by filling up the hole is merge conflicts. I have branches that
regularly require manual conflict resolution because people think they
are clever when they fill up holes.

Simply add your cases to the end of the file, okay? (And up to 078 is
taken now, if someone needs to assign a new test number for his patch.)

Kevin

Re: [Qemu-devel] [PATCH V7 3/6] qemu-iotests: add 058 internal snapshot export with qemu-nbd case

[Wenchao Xia]

于 2014/1/16 20:52, Kevin Wolf 写道:
> Am 04.12.2013 um 10:10 hat Wenchao Xia geschrieben:
>> This case can't run when IMGPROTO=nbd, since it needs to create some
>> internal snapshot which would fail for EOF write request, even when
>> TEST_IMG is exported with "-f raw" in common.rc, so set _supported_proto
>> to file.
>>
>> _require_command() is changed to tip what util is missing, instead
>> of printing a blank.
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>
>> --- 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
>
> Please, please, please, stop doing this.
>
> If you see a hole in the numbering of test cases, it is because there
> are yet unmerged patches that use this number. The only thing you
> achieve by filling up the hole is merge conflicts. I have branches that
> regularly require manual conflict resolution because people think they
> are clever when they fill up holes.
>
> Simply add your cases to the end of the file, okay? (And up to 078 is
> taken now, if someone needs to assign a new test number for his patch.)
>
> Kevin
>
   058 is the last number when I patched v1 if I didn't miss patch on
list, but you are right whole in number shouldn't be filled by new
patch.