[Qemu-devel] [PATCH v15 00/21] block: Image locking series

[Qemu-devel] [PATCH v15 00/21] block: Image locking series

[Fam Zheng]

v15: Rework a number of things, especially around what and how lockings are
     done.  [Kevin]
     - Map each permission to a locked byte.
     - Make the new option --force-share-perms, and require read-only=on.
     - Update test case 153 accordingly.
     - Only add -U where necessary in iotest 030.
     - Reject -U if conflicting with image opts in qemu-img/qemu-io.

v14: Replace BDRV_ flag with the "force-shared-write" block option. [Kevin]
     Add bs->force_shared_write.
     Update test case accordingly.
     A few fixes in the locking code spotted by patchew and the new test,
     though the long line error is still there for readability.
     Replace the workaround to drive-backup with a real fix in patch 17.

v13: - Address Max's comments.
     - Add reviewed-by from Max and Eric.
     - Rebase for 2.10:
       * Use op blocker API
       * Add --unsafe-read for qemu-img and qemu-io

Fam Zheng (21):
  block: Make bdrv_perm_names public
  block: Define BLK_PERM_MAX
  block: Add, parse and store "force-share" option
  block: Respect "force-share" in perm propagating
  qemu-img: Add --force-share option to subcommands
  qemu-img: Update documentation for -U
  qemu-io: Add --force-share option
  iotests: 030: Prepare for image locking
  iotests: 046: Prepare for image locking
  iotests: 055: Don't attach the target image already for drive-backup
  iotests: 085: Avoid image locking conflict
  iotests: 087: Don't attach test image twice
  iotests: 091: Quit QEMU before checking image
  iotests: 172: Use separate images for multiple devices
  tests: Use null-co:// instead of /dev/null as the dummy image
  file-posix: Add 'locking' option
  tests: Disable image lock in test-replication
  block: Reuse bs as backing hd for drive-backup sync=none
  osdep: Add qemu_lock_fd and qemu_unlock_fd
  file-posix: Add image locking to perm operations
  qemu-iotests: Add test case 153 for image locking

 block.c                    |  51 ++++--
 block/file-posix.c         | 272 ++++++++++++++++++++++++++++++-
 blockdev.c                 |  15 +-
 include/block/block.h      |   5 +
 include/block/block_int.h  |   1 +
 include/qemu/osdep.h       |   3 +
 qapi/block-core.json       |   3 +
 qemu-img-cmds.hx           |  36 ++---
 qemu-img.c                 | 154 +++++++++++++-----
 qemu-io.c                  |  42 ++++-
 tests/drive_del-test.c     |   2 +-
 tests/nvme-test.c          |   2 +-
 tests/qemu-iotests/030     |  18 +--
 tests/qemu-iotests/046     |   2 +-
 tests/qemu-iotests/055     |  32 ++--
 tests/qemu-iotests/085     |  34 ++--
 tests/qemu-iotests/085.out |   3 +-
 tests/qemu-iotests/087     |   6 +-
 tests/qemu-iotests/091     |   2 +
 tests/qemu-iotests/153     | 220 +++++++++++++++++++++++++
 tests/qemu-iotests/153.out | 390 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/172     |  55 ++++---
 tests/qemu-iotests/172.out |  50 +++---
 tests/qemu-iotests/group   |   1 +
 tests/test-replication.c   |   9 +-
 tests/usb-hcd-uhci-test.c  |   2 +-
 tests/usb-hcd-xhci-test.c  |   2 +-
 tests/virtio-blk-test.c    |   2 +-
 tests/virtio-scsi-test.c   |   5 +-
 util/osdep.c               |  48 ++++++
 30 files changed, 1291 insertions(+), 176 deletions(-)
 create mode 100755 tests/qemu-iotests/153
 create mode 100644 tests/qemu-iotests/153.out

-- 
2.9.3

[Qemu-devel] [PATCH v15 01/21] block: Make bdrv_perm_names public

[Fam Zheng]

It can be used outside of block.c for making user friendly messages.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block.c               | 2 +-
 include/block/block.h | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 1e668fb..fce77bf 100644
--- a/block.c
+++ b/block.c
@@ -1551,7 +1551,7 @@ static char *bdrv_child_user_desc(BdrvChild *c)
     return g_strdup("another user");
 }
 
-static char *bdrv_perm_names(uint64_t perm)
+char *bdrv_perm_names(uint64_t perm)
 {
     struct perm_name {
         uint64_t perm;
diff --git a/include/block/block.h b/include/block/block.h
index 5ddc0cf..eb0565d 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -224,6 +224,8 @@ enum {
     BLK_PERM_ALL                = 0x1f,
 };
 
+char *bdrv_perm_names(uint64_t perm);
+
 /* disk I/O throttling */
 void bdrv_init(void);
 void bdrv_init_with_whitelist(void);
-- 
2.9.3

[Qemu-devel] [PATCH v15 02/21] block: Define BLK_PERM_MAX

[Fam Zheng]

This is the order of the largest possible permission.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 include/block/block.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/block/block.h b/include/block/block.h
index eb0565d..a798f10 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -224,6 +224,8 @@ enum {
     BLK_PERM_ALL                = 0x1f,
 };
 
+#define BLK_PERM_MAX (64 - clz64((uint64_t)BLK_PERM_ALL))
+
 char *bdrv_perm_names(uint64_t perm);
 
 /* disk I/O throttling */
-- 
2.9.3

Re: [Qemu-devel] [PATCH v15 02/21] block: Define BLK_PERM_MAX

[Kevin Wolf]

Am 26.04.2017 um 05:33 hat Fam Zheng geschrieben:
> This is the order of the largest possible permission.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  include/block/block.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/include/block/block.h b/include/block/block.h
> index eb0565d..a798f10 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -224,6 +224,8 @@ enum {
>      BLK_PERM_ALL                = 0x1f,
>  };
>  
> +#define BLK_PERM_MAX (64 - clz64((uint64_t)BLK_PERM_ALL))

Contrary to the commit message, this is the number of permission bits in
use (i.e. one more than the largest possible permission). You're using
it correctly, though, because your loop condition is i < BLK_PERM_MAX.

This could use an updated commit message and a comment at the #define at
least. Ideally a less ambiguous name instead of the commit (because _MAX
seems to imply what the commit message currently says, not what it
really is), but I can't think of one.

Kevin

Re: [Qemu-devel] [PATCH v15 02/21] block: Define BLK_PERM_MAX

[Fam Zheng]

On Wed, 04/26 11:36, Kevin Wolf wrote:
> Am 26.04.2017 um 05:33 hat Fam Zheng geschrieben:
> > This is the order of the largest possible permission.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  include/block/block.h | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/include/block/block.h b/include/block/block.h
> > index eb0565d..a798f10 100644
> > --- a/include/block/block.h
> > +++ b/include/block/block.h
> > @@ -224,6 +224,8 @@ enum {
> >      BLK_PERM_ALL                = 0x1f,
> >  };
> >  
> > +#define BLK_PERM_MAX (64 - clz64((uint64_t)BLK_PERM_ALL))
> 
> Contrary to the commit message, this is the number of permission bits in
> use (i.e. one more than the largest possible permission). You're using
> it correctly, though, because your loop condition is i < BLK_PERM_MAX.
> 
> This could use an updated commit message and a comment at the #define at
> least. Ideally a less ambiguous name instead of the commit (because _MAX
> seems to imply what the commit message currently says, not what it
> really is), but I can't think of one.

Good point.

Given it another thought, using BLK_PERM_ALL in the loop condition is as easy.
I'll drop this patch.

Fam

[Qemu-devel] [PATCH v15 03/21] block: Add, parse and store "force-share" option

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block.c                   | 17 +++++++++++++++++
 include/block/block.h     |  1 +
 include/block/block_int.h |  1 +
 qapi/block-core.json      |  3 +++
 4 files changed, 22 insertions(+)

diff --git a/block.c b/block.c
index fce77bf..9db39b6 100644
--- a/block.c
+++ b/block.c
@@ -763,6 +763,7 @@ static void bdrv_inherited_options(int *child_flags, QDict *child_options,
      * the parent. */
     qdict_copy_default(child_options, parent_options, BDRV_OPT_CACHE_DIRECT);
     qdict_copy_default(child_options, parent_options, BDRV_OPT_CACHE_NO_FLUSH);
+    qdict_copy_default(child_options, parent_options, BDRV_OPT_FORCE_SHARE);
 
     /* Inherit the read-only option from the parent if it's not set */
     qdict_copy_default(child_options, parent_options, BDRV_OPT_READ_ONLY);
@@ -871,6 +872,7 @@ static void bdrv_backing_options(int *child_flags, QDict *child_options,
      * which is only applied on the top level (BlockBackend) */
     qdict_copy_default(child_options, parent_options, BDRV_OPT_CACHE_DIRECT);
     qdict_copy_default(child_options, parent_options, BDRV_OPT_CACHE_NO_FLUSH);
+    qdict_copy_default(child_options, parent_options, BDRV_OPT_FORCE_SHARE);
 
     /* backing files always opened read-only */
     qdict_set_default_str(child_options, BDRV_OPT_READ_ONLY, "on");
@@ -1115,6 +1117,11 @@ QemuOptsList bdrv_runtime_opts = {
             .type = QEMU_OPT_STRING,
             .help = "discard operation (ignore/off, unmap/on)",
         },
+        {
+            .name = BDRV_OPT_FORCE_SHARE,
+            .type = QEMU_OPT_BOOL,
+            .help = "always accept other writers (default: off)",
+        },
         { /* end of list */ }
     },
 };
@@ -1154,6 +1161,16 @@ static int bdrv_open_common(BlockDriverState *bs, BlockBackend *file,
     drv = bdrv_find_format(driver_name);
     assert(drv != NULL);
 
+    bs->force_share = qemu_opt_get_bool(opts, BDRV_OPT_FORCE_SHARE, false);
+
+    if (bs->force_share && (bs->open_flags & BDRV_O_RDWR)) {
+        error_setg(errp,
+                   BDRV_OPT_FORCE_SHARE
+                   "=on can only be used with read-only images");
+        ret = -EINVAL;
+        goto fail_opts;
+    }
+
     if (file != NULL) {
         filename = blk_bs(file)->filename;
     } else {
diff --git a/include/block/block.h b/include/block/block.h
index a798f10..feae379 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -109,6 +109,7 @@ typedef struct HDGeometry {
 #define BDRV_OPT_CACHE_NO_FLUSH "cache.no-flush"
 #define BDRV_OPT_READ_ONLY      "read-only"
 #define BDRV_OPT_DISCARD        "discard"
+#define BDRV_OPT_FORCE_SHARE    "force-share"
 
 
 #define BDRV_SECTOR_BITS   9
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 59400bd..94ba697 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -518,6 +518,7 @@ struct BlockDriverState {
     bool valid_key; /* if true, a valid encryption key has been set */
     bool sg;        /* if true, the device is a /dev/sg* */
     bool probed;    /* if true, format was probed rather than specified */
+    bool force_share; /* if true, always allow all shared permissions */
 
     BlockDriver *drv; /* NULL means no media */
     void *opaque;
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 033457c..5bc56969 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2880,6 +2880,8 @@
 #                 (default: false)
 # @detect-zeroes: detect and optimize zero writes (Since 2.1)
 #                 (default: off)
+# @force-share:   force share all permission on added nodes.
+#                 Requires read-only=true. (Since 2.10)
 #
 # Remaining options are determined by the block driver.
 #
@@ -2891,6 +2893,7 @@
             '*discard': 'BlockdevDiscardOptions',
             '*cache': 'BlockdevCacheOptions',
             '*read-only': 'bool',
+            '*force-share': 'bool',
             '*detect-zeroes': 'BlockdevDetectZeroesOptions' },
   'discriminator': 'driver',
   'data': {
-- 
2.9.3

[Qemu-devel] [PATCH v15 04/21] block: Respect "force-share" in perm propagating

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block.c | 32 ++++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/block.c b/block.c
index 9db39b6..e9f4750 100644
--- a/block.c
+++ b/block.c
@@ -1430,6 +1430,22 @@ static int bdrv_child_check_perm(BdrvChild *c, uint64_t perm, uint64_t shared,
 static void bdrv_child_abort_perm_update(BdrvChild *c);
 static void bdrv_child_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared);
 
+static void bdrv_child_perm(BlockDriverState *bs, BlockDriverState *child_bs,
+                            BdrvChild *c,
+                            const BdrvChildRole *role,
+                            uint64_t parent_perm, uint64_t parent_shared,
+                            uint64_t *nperm, uint64_t *nshared)
+{
+    if (bs->drv && bs->drv->bdrv_child_perm) {
+        bs->drv->bdrv_child_perm(bs, c, role,
+                                 parent_perm, parent_shared,
+                                 nperm, nshared);
+    }
+    if (child_bs && child_bs->force_share) {
+        *nshared = BLK_PERM_ALL;
+    }
+}
+
 /*
  * Check whether permissions on this node can be changed in a way that
  * @cumulative_perms and @cumulative_shared_perms are the new cumulative
@@ -1474,9 +1490,9 @@ static int bdrv_check_perm(BlockDriverState *bs, uint64_t cumulative_perms,
     /* Check all children */
     QLIST_FOREACH(c, &bs->children, next) {
         uint64_t cur_perm, cur_shared;
-        drv->bdrv_child_perm(bs, c, c->role,
-                             cumulative_perms, cumulative_shared_perms,
-                             &cur_perm, &cur_shared);
+        bdrv_child_perm(bs, c->bs, c, c->role,
+                        cumulative_perms, cumulative_shared_perms,
+                        &cur_perm, &cur_shared);
         ret = bdrv_child_check_perm(c, cur_perm, cur_shared, ignore_children,
                                     errp);
         if (ret < 0) {
@@ -1536,9 +1552,9 @@ static void bdrv_set_perm(BlockDriverState *bs, uint64_t cumulative_perms,
     /* Update all children */
     QLIST_FOREACH(c, &bs->children, next) {
         uint64_t cur_perm, cur_shared;
-        drv->bdrv_child_perm(bs, c, c->role,
-                             cumulative_perms, cumulative_shared_perms,
-                             &cur_perm, &cur_shared);
+        bdrv_child_perm(bs, c->bs, c, c->role,
+                        cumulative_perms, cumulative_shared_perms,
+                        &cur_perm, &cur_shared);
         bdrv_child_set_perm(c, cur_perm, cur_shared);
     }
 }
@@ -1873,8 +1889,8 @@ BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
 
     assert(parent_bs->drv);
     assert(bdrv_get_aio_context(parent_bs) == bdrv_get_aio_context(child_bs));
-    parent_bs->drv->bdrv_child_perm(parent_bs, NULL, child_role,
-                                    perm, shared_perm, &perm, &shared_perm);
+    bdrv_child_perm(parent_bs, child_bs, NULL, child_role,
+                    perm, shared_perm, &perm, &shared_perm);
 
     child = bdrv_root_attach_child(child_bs, child_name, child_role,
                                    perm, shared_perm, parent_bs, errp);
-- 
2.9.3

[Qemu-devel] [PATCH v15 05/21] qemu-img: Add --force-share option to subcommands

[Fam Zheng]

This will force the opened images to allow sharing all permissions with other
programs.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 qemu-img.c | 154 ++++++++++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 118 insertions(+), 36 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index ed24371..c0fb1ce 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -28,6 +28,7 @@
 #include "qapi/qobject-output-visitor.h"
 #include "qapi/qmp/qerror.h"
 #include "qapi/qmp/qjson.h"
+#include "qapi/qmp/qbool.h"
 #include "qemu/cutils.h"
 #include "qemu/config-file.h"
 #include "qemu/option.h"
@@ -283,12 +284,20 @@ static int img_open_password(BlockBackend *blk, const char *filename,
 
 static BlockBackend *img_open_opts(const char *optstr,
                                    QemuOpts *opts, int flags, bool writethrough,
-                                   bool quiet)
+                                   bool quiet, bool force_share)
 {
     QDict *options;
     Error *local_err = NULL;
     BlockBackend *blk;
     options = qemu_opts_to_qdict(opts, NULL);
+    if (force_share) {
+        if (qdict_haskey(options, BDRV_OPT_FORCE_SHARE)
+            && !qdict_get_bool(options, BDRV_OPT_FORCE_SHARE)) {
+            error_report("--force-share/-U conflicts with image options");
+            return NULL;
+        }
+        qdict_put(options, BDRV_OPT_FORCE_SHARE, qbool_from_bool(true));
+    }
     blk = blk_new_open(NULL, NULL, options, flags, &local_err);
     if (!blk) {
         error_reportf_err(local_err, "Could not open '%s': ", optstr);
@@ -305,17 +314,20 @@ static BlockBackend *img_open_opts(const char *optstr,
 
 static BlockBackend *img_open_file(const char *filename,
                                    const char *fmt, int flags,
-                                   bool writethrough, bool quiet)
+                                   bool writethrough, bool quiet,
+                                   bool force_share)
 {
     BlockBackend *blk;
     Error *local_err = NULL;
-    QDict *options = NULL;
+    QDict *options = qdict_new();
 
     if (fmt) {
-        options = qdict_new();
         qdict_put(options, "driver", qstring_from_str(fmt));
     }
 
+    if (force_share) {
+        qdict_put(options, BDRV_OPT_FORCE_SHARE, qbool_from_bool(true));
+    }
     blk = blk_new_open(filename, NULL, options, flags, &local_err);
     if (!blk) {
         error_reportf_err(local_err, "Could not open '%s': ", filename);
@@ -334,7 +346,7 @@ static BlockBackend *img_open_file(const char *filename,
 static BlockBackend *img_open(bool image_opts,
                               const char *filename,
                               const char *fmt, int flags, bool writethrough,
-                              bool quiet)
+                              bool quiet, bool force_share)
 {
     BlockBackend *blk;
     if (image_opts) {
@@ -348,9 +360,11 @@ static BlockBackend *img_open(bool image_opts,
         if (!opts) {
             return NULL;
         }
-        blk = img_open_opts(filename, opts, flags, writethrough, quiet);
+        blk = img_open_opts(filename, opts, flags, writethrough, quiet,
+                            force_share);
     } else {
-        blk = img_open_file(filename, fmt, flags, writethrough, quiet);
+        blk = img_open_file(filename, fmt, flags, writethrough, quiet,
+                            force_share);
     }
     return blk;
 }
@@ -650,6 +664,7 @@ static int img_check(int argc, char **argv)
     ImageCheck *check;
     bool quiet = false;
     bool image_opts = false;
+    bool force_share = false;
 
     fmt = NULL;
     output = NULL;
@@ -664,9 +679,10 @@ static int img_check(int argc, char **argv)
             {"output", required_argument, 0, OPTION_OUTPUT},
             {"object", required_argument, 0, OPTION_OBJECT},
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
+            {"force-share", no_argument, 0, 'U'},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, ":hf:r:T:q",
+        c = getopt_long(argc, argv, ":hf:r:T:qU",
                         long_options, &option_index);
         if (c == -1) {
             break;
@@ -705,6 +721,9 @@ static int img_check(int argc, char **argv)
         case 'q':
             quiet = true;
             break;
+        case 'U':
+            force_share = true;
+            break;
         case OPTION_OBJECT: {
             QemuOpts *opts;
             opts = qemu_opts_parse_noisily(&qemu_object_opts,
@@ -744,7 +763,8 @@ static int img_check(int argc, char **argv)
         return 1;
     }
 
-    blk = img_open(image_opts, filename, fmt, flags, writethrough, quiet);
+    blk = img_open(image_opts, filename, fmt, flags, writethrough, quiet,
+                   force_share);
     if (!blk) {
         return 1;
     }
@@ -947,7 +967,8 @@ static int img_commit(int argc, char **argv)
         return 1;
     }
 
-    blk = img_open(image_opts, filename, fmt, flags, writethrough, quiet);
+    blk = img_open(image_opts, filename, fmt, flags, writethrough, quiet,
+                   false);
     if (!blk) {
         return 1;
     }
@@ -1206,6 +1227,7 @@ static int img_compare(int argc, char **argv)
     int c, pnum;
     uint64_t progress_base;
     bool image_opts = false;
+    bool force_share = false;
 
     cache = BDRV_DEFAULT_CACHE;
     for (;;) {
@@ -1213,9 +1235,10 @@ static int img_compare(int argc, char **argv)
             {"help", no_argument, 0, 'h'},
             {"object", required_argument, 0, OPTION_OBJECT},
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
+            {"force-share", no_argument, 0, 'U'},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, ":hf:F:T:pqs",
+        c = getopt_long(argc, argv, ":hf:F:T:pqsU",
                         long_options, NULL);
         if (c == -1) {
             break;
@@ -1248,6 +1271,9 @@ static int img_compare(int argc, char **argv)
         case 's':
             strict = true;
             break;
+        case 'U':
+            force_share = true;
+            break;
         case OPTION_OBJECT: {
             QemuOpts *opts;
             opts = qemu_opts_parse_noisily(&qemu_object_opts,
@@ -1293,13 +1319,15 @@ static int img_compare(int argc, char **argv)
         goto out3;
     }
 
-    blk1 = img_open(image_opts, filename1, fmt1, flags, writethrough, quiet);
+    blk1 = img_open(image_opts, filename1, fmt1, flags, writethrough, quiet,
+                    force_share);
     if (!blk1) {
         ret = 2;
         goto out3;
     }
 
-    blk2 = img_open(image_opts, filename2, fmt2, flags, writethrough, quiet);
+    blk2 = img_open(image_opts, filename2, fmt2, flags, writethrough, quiet,
+                    force_share);
     if (!blk2) {
         ret = 2;
         goto out2;
@@ -1934,6 +1962,7 @@ static int img_convert(int argc, char **argv)
     bool writethrough, src_writethrough, quiet = false, image_opts = false,
          compress = false, skip_create = false, progress = false;
     int64_t ret = -EINVAL;
+    bool force_share = false;
 
     ImgConvertState s = (ImgConvertState) {
         /* Need at least 4k of zeros for sparse detection */
@@ -1948,9 +1977,10 @@ static int img_convert(int argc, char **argv)
             {"help", no_argument, 0, 'h'},
             {"object", required_argument, 0, OPTION_OBJECT},
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
+            {"force-share", no_argument, 0, 'U'},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, ":hf:O:B:ce6o:s:l:S:pt:T:qnm:W",
+        c = getopt_long(argc, argv, ":hf:O:B:ce6o:s:l:S:pt:T:qnm:WU",
                         long_options, NULL);
         if (c == -1) {
             break;
@@ -2054,6 +2084,9 @@ static int img_convert(int argc, char **argv)
         case 'W':
             s.wr_in_order = false;
             break;
+        case 'U':
+            force_share = true;
+            break;
         case OPTION_OBJECT:
             opts = qemu_opts_parse_noisily(&qemu_object_opts,
                                            optarg, true);
@@ -2117,7 +2150,8 @@ static int img_convert(int argc, char **argv)
 
     for (bs_i = 0; bs_i < s.src_num; bs_i++) {
         s.src[bs_i] = img_open(image_opts, argv[optind + bs_i],
-                                fmt, src_flags, src_writethrough, quiet);
+                               fmt, src_flags, src_writethrough, quiet,
+                               force_share);
         if (!s.src[bs_i]) {
             ret = -1;
             goto out;
@@ -2262,7 +2296,8 @@ static int img_convert(int argc, char **argv)
      * the bdrv_create() call which takes different params.
      * Not critical right now, so fix can wait...
      */
-    s.target = img_open_file(out_filename, out_fmt, flags, writethrough, quiet);
+    s.target = img_open_file(out_filename, out_fmt, flags, writethrough, quiet,
+                             false);
     if (!s.target) {
         ret = -1;
         goto out;
@@ -2413,7 +2448,7 @@ static gboolean str_equal_func(gconstpointer a, gconstpointer b)
 static ImageInfoList *collect_image_info_list(bool image_opts,
                                               const char *filename,
                                               const char *fmt,
-                                              bool chain)
+                                              bool chain, bool force_share)
 {
     ImageInfoList *head = NULL;
     ImageInfoList **last = &head;
@@ -2436,7 +2471,8 @@ static ImageInfoList *collect_image_info_list(bool image_opts,
         g_hash_table_insert(filenames, (gpointer)filename, NULL);
 
         blk = img_open(image_opts, filename, fmt,
-                       BDRV_O_NO_BACKING | BDRV_O_NO_IO, false, false);
+                       BDRV_O_NO_BACKING | BDRV_O_NO_IO, false, false,
+                       force_share);
         if (!blk) {
             goto err;
         }
@@ -2488,6 +2524,7 @@ static int img_info(int argc, char **argv)
     const char *filename, *fmt, *output;
     ImageInfoList *list;
     bool image_opts = false;
+    bool force_share = false;
 
     fmt = NULL;
     output = NULL;
@@ -2500,9 +2537,10 @@ static int img_info(int argc, char **argv)
             {"backing-chain", no_argument, 0, OPTION_BACKING_CHAIN},
             {"object", required_argument, 0, OPTION_OBJECT},
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
+            {"force-share", no_argument, 0, 'U'},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, ":f:h",
+        c = getopt_long(argc, argv, ":f:hU",
                         long_options, &option_index);
         if (c == -1) {
             break;
@@ -2520,6 +2558,9 @@ static int img_info(int argc, char **argv)
         case 'f':
             fmt = optarg;
             break;
+        case 'U':
+            force_share = true;
+            break;
         case OPTION_OUTPUT:
             output = optarg;
             break;
@@ -2559,7 +2600,8 @@ static int img_info(int argc, char **argv)
         return 1;
     }
 
-    list = collect_image_info_list(image_opts, filename, fmt, chain);
+    list = collect_image_info_list(image_opts, filename, fmt, chain,
+                                   force_share);
     if (!list) {
         return 1;
     }
@@ -2705,6 +2747,7 @@ static int img_map(int argc, char **argv)
     MapEntry curr = { .length = 0 }, next;
     int ret = 0;
     bool image_opts = false;
+    bool force_share = false;
 
     fmt = NULL;
     output = NULL;
@@ -2716,9 +2759,10 @@ static int img_map(int argc, char **argv)
             {"output", required_argument, 0, OPTION_OUTPUT},
             {"object", required_argument, 0, OPTION_OBJECT},
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
+            {"force-share", no_argument, 0, 'U'},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, ":f:h",
+        c = getopt_long(argc, argv, ":f:hU",
                         long_options, &option_index);
         if (c == -1) {
             break;
@@ -2736,6 +2780,9 @@ static int img_map(int argc, char **argv)
         case 'f':
             fmt = optarg;
             break;
+        case 'U':
+            force_share = true;
+            break;
         case OPTION_OUTPUT:
             output = optarg;
             break;
@@ -2772,7 +2819,7 @@ static int img_map(int argc, char **argv)
         return 1;
     }
 
-    blk = img_open(image_opts, filename, fmt, 0, false, false);
+    blk = img_open(image_opts, filename, fmt, 0, false, false, force_share);
     if (!blk) {
         return 1;
     }
@@ -2835,6 +2882,7 @@ static int img_snapshot(int argc, char **argv)
     bool quiet = false;
     Error *err = NULL;
     bool image_opts = false;
+    bool force_share = false;
 
     bdrv_oflags = BDRV_O_RDWR;
     /* Parse commandline parameters */
@@ -2843,9 +2891,10 @@ static int img_snapshot(int argc, char **argv)
             {"help", no_argument, 0, 'h'},
             {"object", required_argument, 0, OPTION_OBJECT},
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
+            {"force-share", no_argument, 0, 'U'},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, ":la:c:d:hq",
+        c = getopt_long(argc, argv, ":la:c:d:hqU",
                         long_options, NULL);
         if (c == -1) {
             break;
@@ -2895,6 +2944,9 @@ static int img_snapshot(int argc, char **argv)
         case 'q':
             quiet = true;
             break;
+        case 'U':
+            force_share = true;
+            break;
         case OPTION_OBJECT: {
             QemuOpts *opts;
             opts = qemu_opts_parse_noisily(&qemu_object_opts,
@@ -2921,7 +2973,8 @@ static int img_snapshot(int argc, char **argv)
     }
 
     /* Open the image */
-    blk = img_open(image_opts, filename, NULL, bdrv_oflags, false, quiet);
+    blk = img_open(image_opts, filename, NULL, bdrv_oflags, false, quiet,
+                   force_share);
     if (!blk) {
         return 1;
     }
@@ -2985,6 +3038,7 @@ static int img_rebase(int argc, char **argv)
     int c, flags, src_flags, ret;
     bool writethrough, src_writethrough;
     int unsafe = 0;
+    bool force_share = false;
     int progress = 0;
     bool quiet = false;
     Error *local_err = NULL;
@@ -3001,9 +3055,10 @@ static int img_rebase(int argc, char **argv)
             {"help", no_argument, 0, 'h'},
             {"object", required_argument, 0, OPTION_OBJECT},
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
+            {"force-share", no_argument, 0, 'U'},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, ":hf:F:b:upt:T:q",
+        c = getopt_long(argc, argv, ":hf:F:b:upt:T:qU",
                         long_options, NULL);
         if (c == -1) {
             break;
@@ -3053,6 +3108,9 @@ static int img_rebase(int argc, char **argv)
         case OPTION_IMAGE_OPTS:
             image_opts = true;
             break;
+        case 'U':
+            force_share = true;
+            break;
         }
     }
 
@@ -3101,7 +3159,8 @@ static int img_rebase(int argc, char **argv)
      * Ignore the old backing file for unsafe rebase in case we want to correct
      * the reference to a renamed or moved backing file.
      */
-    blk = img_open(image_opts, filename, fmt, flags, writethrough, quiet);
+    blk = img_open(image_opts, filename, fmt, flags, writethrough, quiet,
+                   false);
     if (!blk) {
         ret = -1;
         goto out;
@@ -3126,6 +3185,13 @@ static int img_rebase(int argc, char **argv)
             qdict_put(options, "driver", qstring_from_str(bs->backing_format));
         }
 
+        if (force_share) {
+            if (!options) {
+                options = qdict_new();
+            }
+            qdict_put(options, BDRV_OPT_FORCE_SHARE,
+                      qbool_from_bool(true));
+        }
         bdrv_get_backing_filename(bs, backing_name, sizeof(backing_name));
         blk_old_backing = blk_new_open(backing_name, NULL,
                                        options, src_flags, &local_err);
@@ -3138,11 +3204,13 @@ static int img_rebase(int argc, char **argv)
         }
 
         if (out_baseimg[0]) {
+            options = qdict_new();
             if (out_basefmt) {
-                options = qdict_new();
                 qdict_put(options, "driver", qstring_from_str(out_basefmt));
-            } else {
-                options = NULL;
+            }
+            if (force_share) {
+                qdict_put(options, BDRV_OPT_FORCE_SHARE,
+                          qbool_from_bool(true));
             }
 
             blk_new_backing = blk_new_open(out_baseimg, NULL,
@@ -3448,7 +3516,8 @@ static int img_resize(int argc, char **argv)
     qemu_opts_del(param);
 
     blk = img_open(image_opts, filename, fmt,
-                   BDRV_O_RDWR | BDRV_O_RESIZE, false, quiet);
+                   BDRV_O_RDWR | BDRV_O_RESIZE, false, quiet,
+                   false);
     if (!blk) {
         ret = -1;
         goto out;
@@ -3611,7 +3680,8 @@ static int img_amend(int argc, char **argv)
         goto out;
     }
 
-    blk = img_open(image_opts, filename, fmt, flags, writethrough, quiet);
+    blk = img_open(image_opts, filename, fmt, flags, writethrough, quiet,
+                   false);
     if (!blk) {
         ret = -1;
         goto out;
@@ -3779,6 +3849,7 @@ static int img_bench(int argc, char **argv)
     bool writethrough = false;
     struct timeval t1, t2;
     int i;
+    bool force_share = false;
 
     for (;;) {
         static const struct option long_options[] = {
@@ -3787,9 +3858,10 @@ static int img_bench(int argc, char **argv)
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {"pattern", required_argument, 0, OPTION_PATTERN},
             {"no-drain", no_argument, 0, OPTION_NO_DRAIN},
+            {"force-share", no_argument, 0, 'U'},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, ":hc:d:f:no:qs:S:t:w", long_options, NULL);
+        c = getopt_long(argc, argv, ":hc:d:f:no:qs:S:t:wU", long_options, NULL);
         if (c == -1) {
             break;
         }
@@ -3883,6 +3955,9 @@ static int img_bench(int argc, char **argv)
             flags |= BDRV_O_RDWR;
             is_write = true;
             break;
+        case 'U':
+            force_share = true;
+            break;
         case OPTION_PATTERN:
         {
             unsigned long res;
@@ -3930,7 +4005,8 @@ static int img_bench(int argc, char **argv)
         goto out;
     }
 
-    blk = img_open(image_opts, filename, fmt, flags, writethrough, quiet);
+    blk = img_open(image_opts, filename, fmt, flags, writethrough, quiet,
+                   force_share);
     if (!blk) {
         ret = -1;
         goto out;
@@ -4097,6 +4173,7 @@ static int img_dd(int argc, char **argv)
     const char *fmt = NULL;
     int64_t size = 0;
     int64_t block_count = 0, out_pos, in_pos;
+    bool force_share = false;
     struct DdInfo dd = {
         .flags = 0,
         .count = 0,
@@ -4125,10 +4202,11 @@ static int img_dd(int argc, char **argv)
     const struct option long_options[] = {
         { "help", no_argument, 0, 'h'},
         { "image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
+        { "force-share", no_argument, 0, 'U'},
         { 0, 0, 0, 0 }
     };
 
-    while ((c = getopt_long(argc, argv, ":hf:O:", long_options, NULL))) {
+    while ((c = getopt_long(argc, argv, ":hf:O:U", long_options, NULL))) {
         if (c == EOF) {
             break;
         }
@@ -4148,6 +4226,9 @@ static int img_dd(int argc, char **argv)
         case 'h':
             help();
             break;
+        case 'U':
+            force_share = true;
+            break;
         case OPTION_IMAGE_OPTS:
             image_opts = true;
             break;
@@ -4192,7 +4273,8 @@ static int img_dd(int argc, char **argv)
         ret = -1;
         goto out;
     }
-    blk1 = img_open(image_opts, in.filename, fmt, 0, false, false);
+    blk1 = img_open(image_opts, in.filename, fmt, 0, false, false,
+                    force_share);
 
     if (!blk1) {
         ret = -1;
@@ -4260,7 +4342,7 @@ static int img_dd(int argc, char **argv)
     }
 
     blk2 = img_open(image_opts, out.filename, out_fmt, BDRV_O_RDWR,
-                    false, false);
+                    false, false, false);
 
     if (!blk2) {
         ret = -1;
-- 
2.9.3

[Qemu-devel] [PATCH v15 06/21] qemu-img: Update documentation for -U

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 qemu-img-cmds.hx | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index 8ac7822..ae309c0 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -10,15 +10,15 @@ STEXI
 ETEXI
 
 DEF("bench", img_bench,
-    "bench [-c count] [-d depth] [-f fmt] [--flush-interval=flush_interval] [-n] [--no-drain] [-o offset] [--pattern=pattern] [-q] [-s buffer_size] [-S step_size] [-t cache] [-w] filename")
+    "bench [-c count] [-d depth] [-f fmt] [--flush-interval=flush_interval] [-n] [--no-drain] [-o offset] [--pattern=pattern] [-q] [-s buffer_size] [-S step_size] [-t cache] [-w] [-U] filename")
 STEXI
-@item bench [-c @var{count}] [-d @var{depth}] [-f @var{fmt}] [--flush-interval=@var{flush_interval}] [-n] [--no-drain] [-o @var{offset}] [--pattern=@var{pattern}] [-q] [-s @var{buffer_size}] [-S @var{step_size}] [-t @var{cache}] [-w] @var{filename}
+@item bench [-c @var{count}] [-d @var{depth}] [-f @var{fmt}] [--flush-interval=@var{flush_interval}] [-n] [--no-drain] [-o @var{offset}] [--pattern=@var{pattern}] [-q] [-s @var{buffer_size}] [-S @var{step_size}] [-t @var{cache}] [-w] [-U] @var{filename}
 ETEXI
 
 DEF("check", img_check,
-    "check [-q] [--object objectdef] [--image-opts] [-f fmt] [--output=ofmt] [-r [leaks | all]] [-T src_cache] filename")
+    "check [-q] [--object objectdef] [--image-opts] [-f fmt] [--output=ofmt] [-r [leaks | all]] [-T src_cache] [-U] filename")
 STEXI
-@item check [--object @var{objectdef}] [--image-opts] [-q] [-f @var{fmt}] [--output=@var{ofmt}] [-r [leaks | all]] [-T @var{src_cache}] @var{filename}
+@item check [--object @var{objectdef}] [--image-opts] [-q] [-f @var{fmt}] [--output=@var{ofmt}] [-r [leaks | all]] [-T @var{src_cache}] [-U] @var{filename}
 ETEXI
 
 DEF("create", img_create,
@@ -34,45 +34,45 @@ STEXI
 ETEXI
 
 DEF("compare", img_compare,
-    "compare [--object objectdef] [--image-opts] [-f fmt] [-F fmt] [-T src_cache] [-p] [-q] [-s] filename1 filename2")
+    "compare [--object objectdef] [--image-opts] [-f fmt] [-F fmt] [-T src_cache] [-p] [-q] [-s] [-U] filename1 filename2")
 STEXI
-@item compare [--object @var{objectdef}] [--image-opts] [-f @var{fmt}] [-F @var{fmt}] [-T @var{src_cache}] [-p] [-q] [-s] @var{filename1} @var{filename2}
+@item compare [--object @var{objectdef}] [--image-opts] [-f @var{fmt}] [-F @var{fmt}] [-T @var{src_cache}] [-p] [-q] [-s] [-U] @var{filename1} @var{filename2}
 ETEXI
 
 DEF("convert", img_convert,
-    "convert [--object objectdef] [--image-opts] [-c] [-p] [-q] [-n] [-f fmt] [-t cache] [-T src_cache] [-O output_fmt] [-o options] [-s snapshot_id_or_name] [-l snapshot_param] [-S sparse_size] [-m num_coroutines] [-W] filename [filename2 [...]] output_filename")
+    "convert [--object objectdef] [--image-opts] [-c] [-p] [-q] [-n] [-f fmt] [-t cache] [-T src_cache] [-O output_fmt] [-o options] [-s snapshot_id_or_name] [-l snapshot_param] [-S sparse_size] [-m num_coroutines] [-W] [-U] filename [filename2 [...]] output_filename")
 STEXI
-@item convert [--object @var{objectdef}] [--image-opts] [-c] [-p] [-q] [-n] [-f @var{fmt}] [-t @var{cache}] [-T @var{src_cache}] [-O @var{output_fmt}] [-o @var{options}] [-s @var{snapshot_id_or_name}] [-l @var{snapshot_param}] [-S @var{sparse_size}] [-m @var{num_coroutines}] [-W] @var{filename} [@var{filename2} [...]] @var{output_filename}
+@item convert [--object @var{objectdef}] [--image-opts] [-c] [-p] [-q] [-n] [-f @var{fmt}] [-t @var{cache}] [-T @var{src_cache}] [-O @var{output_fmt}] [-o @var{options}] [-s @var{snapshot_id_or_name}] [-l @var{snapshot_param}] [-S @var{sparse_size}] [-m @var{num_coroutines}] [-W] [-U] @var{filename} [@var{filename2} [...]] @var{output_filename}
 ETEXI
 
 DEF("dd", img_dd,
-    "dd [--image-opts] [-f fmt] [-O output_fmt] [bs=block_size] [count=blocks] [skip=blocks] if=input of=output")
+    "dd [--image-opts] [-U] [-f fmt] [-O output_fmt] [bs=block_size] [count=blocks] [skip=blocks] if=input of=output")
 STEXI
-@item dd [--image-opts] [-f @var{fmt}] [-O @var{output_fmt}] [bs=@var{block_size}] [count=@var{blocks}] [skip=@var{blocks}] if=@var{input} of=@var{output}
+@item dd [--image-opts] [-U] [-f @var{fmt}] [-O @var{output_fmt}] [bs=@var{block_size}] [count=@var{blocks}] [skip=@var{blocks}] if=@var{input} of=@var{output}
 ETEXI
 
 DEF("info", img_info,
-    "info [--object objectdef] [--image-opts] [-f fmt] [--output=ofmt] [--backing-chain] filename")
+    "info [--object objectdef] [--image-opts] [-f fmt] [--output=ofmt] [--backing-chain] [-U] filename")
 STEXI
-@item info [--object @var{objectdef}] [--image-opts] [-f @var{fmt}] [--output=@var{ofmt}] [--backing-chain] @var{filename}
+@item info [--object @var{objectdef}] [--image-opts] [-f @var{fmt}] [--output=@var{ofmt}] [--backing-chain] [-U] @var{filename}
 ETEXI
 
 DEF("map", img_map,
-    "map [--object objectdef] [--image-opts] [-f fmt] [--output=ofmt] filename")
+    "map [--object objectdef] [--image-opts] [-f fmt] [--output=ofmt] [-U] filename")
 STEXI
-@item map [--object @var{objectdef}] [--image-opts] [-f @var{fmt}] [--output=@var{ofmt}] @var{filename}
+@item map [--object @var{objectdef}] [--image-opts] [-f @var{fmt}] [--output=@var{ofmt}] [-U] @var{filename}
 ETEXI
 
 DEF("snapshot", img_snapshot,
-    "snapshot [--object objectdef] [--image-opts] [-q] [-l | -a snapshot | -c snapshot | -d snapshot] filename")
+    "snapshot [--object objectdef] [--image-opts] [-U] [-q] [-l | -a snapshot | -c snapshot | -d snapshot] filename")
 STEXI
-@item snapshot [--object @var{objectdef}] [--image-opts] [-q] [-l | -a @var{snapshot} | -c @var{snapshot} | -d @var{snapshot}] @var{filename}
+@item snapshot [--object @var{objectdef}] [--image-opts] [-U] [-q] [-l | -a @var{snapshot} | -c @var{snapshot} | -d @var{snapshot}] @var{filename}
 ETEXI
 
 DEF("rebase", img_rebase,
-    "rebase [--object objectdef] [--image-opts] [-q] [-f fmt] [-t cache] [-T src_cache] [-p] [-u] -b backing_file [-F backing_fmt] filename")
+    "rebase [--object objectdef] [--image-opts] [-U] [-q] [-f fmt] [-t cache] [-T src_cache] [-p] [-u] -b backing_file [-F backing_fmt] filename")
 STEXI
-@item rebase [--object @var{objectdef}] [--image-opts] [-q] [-f @var{fmt}] [-t @var{cache}] [-T @var{src_cache}] [-p] [-u] -b @var{backing_file} [-F @var{backing_fmt}] @var{filename}
+@item rebase [--object @var{objectdef}] [--image-opts] [-U] [-q] [-f @var{fmt}] [-t @var{cache}] [-T @var{src_cache}] [-p] [-u] -b @var{backing_file} [-F @var{backing_fmt}] @var{filename}
 ETEXI
 
 DEF("resize", img_resize,
-- 
2.9.3

[Qemu-devel] [PATCH v15 07/21] qemu-io: Add --force-share option

[Fam Zheng]

Add --force-share/-U to program options and -U to open subcommand.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 qemu-io.c | 42 ++++++++++++++++++++++++++++++++++--------
 1 file changed, 34 insertions(+), 8 deletions(-)

diff --git a/qemu-io.c b/qemu-io.c
index 427cbae..cf4b876 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -20,6 +20,7 @@
 #include "qemu/readline.h"
 #include "qemu/log.h"
 #include "qapi/qmp/qstring.h"
+#include "qapi/qmp/qbool.h"
 #include "qom/object_interfaces.h"
 #include "sysemu/block-backend.h"
 #include "block/block_int.h"
@@ -53,7 +54,8 @@ static const cmdinfo_t close_cmd = {
     .oneline    = "close the current open file",
 };
 
-static int openfile(char *name, int flags, bool writethrough, QDict *opts)
+static int openfile(char *name, int flags, bool writethrough, bool force_share,
+                    QDict *opts)
 {
     Error *local_err = NULL;
     BlockDriverState *bs;
@@ -64,6 +66,18 @@ static int openfile(char *name, int flags, bool writethrough, QDict *opts)
         return 1;
     }
 
+    if (force_share) {
+        if (!opts) {
+            opts = qdict_new();
+        }
+        if (qdict_haskey(opts, BDRV_OPT_FORCE_SHARE)
+            && !qdict_get_bool(opts, BDRV_OPT_FORCE_SHARE)) {
+            error_report("-U conflicts with image options");
+            QDECREF(opts);
+            return 1;
+        }
+        qdict_put(opts, BDRV_OPT_FORCE_SHARE, qbool_from_bool(true));
+    }
     qemuio_blk = blk_new_open(name, NULL, opts, flags, &local_err);
     if (!qemuio_blk) {
         error_reportf_err(local_err, "can't open%s%s: ",
@@ -108,6 +122,7 @@ static void open_help(void)
 " -r, -- open file read-only\n"
 " -s, -- use snapshot file\n"
 " -n, -- disable host cache, short for -t none\n"
+" -U, -- force shared permissions\n"
 " -k, -- use kernel AIO implementation (on Linux only)\n"
 " -t, -- use the given cache mode for the image\n"
 " -d, -- use the given discard mode for the image\n"
@@ -124,7 +139,7 @@ static const cmdinfo_t open_cmd = {
     .argmin     = 1,
     .argmax     = -1,
     .flags      = CMD_NOFILE_OK,
-    .args       = "[-rsnk] [-t cache] [-d discard] [-o options] [path]",
+    .args       = "[-rsnkU] [-t cache] [-d discard] [-o options] [path]",
     .oneline    = "open the file specified by path",
     .help       = open_help,
 };
@@ -147,8 +162,9 @@ static int open_f(BlockBackend *blk, int argc, char **argv)
     int c;
     QemuOpts *qopts;
     QDict *opts;
+    bool force_share = false;
 
-    while ((c = getopt(argc, argv, "snro:kt:d:")) != -1) {
+    while ((c = getopt(argc, argv, "snro:kt:d:U")) != -1) {
         switch (c) {
         case 's':
             flags |= BDRV_O_SNAPSHOT;
@@ -188,6 +204,9 @@ static int open_f(BlockBackend *blk, int argc, char **argv)
                 return 0;
             }
             break;
+        case 'U':
+            force_share = true;
+            break;
         default:
             qemu_opts_reset(&empty_opts);
             return qemuio_command_usage(&open_cmd);
@@ -211,9 +230,9 @@ static int open_f(BlockBackend *blk, int argc, char **argv)
     qemu_opts_reset(&empty_opts);
 
     if (optind == argc - 1) {
-        return openfile(argv[optind], flags, writethrough, opts);
+        return openfile(argv[optind], flags, writethrough, force_share, opts);
     } else if (optind == argc) {
-        return openfile(NULL, flags, writethrough, opts);
+        return openfile(NULL, flags, writethrough, force_share, opts);
     } else {
         QDECREF(opts);
         return qemuio_command_usage(&open_cmd);
@@ -257,6 +276,7 @@ static void usage(const char *name)
 "  -T, --trace [[enable=]<pattern>][,events=<file>][,file=<file>]\n"
 "                       specify tracing options\n"
 "                       see qemu-img(1) man page for full description\n"
+"  -U, --force-share    force shared permissions\n"
 "  -h, --help           display this help and exit\n"
 "  -V, --version        output version information and exit\n"
 "\n"
@@ -436,7 +456,7 @@ static QemuOptsList file_opts = {
 int main(int argc, char **argv)
 {
     int readonly = 0;
-    const char *sopt = "hVc:d:f:rsnmkt:T:";
+    const char *sopt = "hVc:d:f:rsnmkt:T:U";
     const struct option lopt[] = {
         { "help", no_argument, NULL, 'h' },
         { "version", no_argument, NULL, 'V' },
@@ -452,6 +472,7 @@ int main(int argc, char **argv)
         { "trace", required_argument, NULL, 'T' },
         { "object", required_argument, NULL, OPTION_OBJECT },
         { "image-opts", no_argument, NULL, OPTION_IMAGE_OPTS },
+        { "force-share", no_argument, 0, 'U'},
         { NULL, 0, NULL, 0 }
     };
     int c;
@@ -462,6 +483,7 @@ int main(int argc, char **argv)
     QDict *opts = NULL;
     const char *format = NULL;
     char *trace_file = NULL;
+    bool force_share = false;
 
 #ifdef CONFIG_POSIX
     signal(SIGPIPE, SIG_IGN);
@@ -524,6 +546,9 @@ int main(int argc, char **argv)
         case 'h':
             usage(progname);
             exit(0);
+        case 'U':
+            force_share = true;
+            break;
         case OPTION_OBJECT: {
             QemuOpts *qopts;
             qopts = qemu_opts_parse_noisily(&qemu_object_opts,
@@ -595,7 +620,7 @@ int main(int argc, char **argv)
                 exit(1);
             }
             opts = qemu_opts_to_qdict(qopts, NULL);
-            if (openfile(NULL, flags, writethrough, opts)) {
+            if (openfile(NULL, flags, writethrough, force_share, opts)) {
                 exit(1);
             }
         } else {
@@ -603,7 +628,8 @@ int main(int argc, char **argv)
                 opts = qdict_new();
                 qdict_put(opts, "driver", qstring_from_str(format));
             }
-            if (openfile(argv[optind], flags, writethrough, opts)) {
+            if (openfile(argv[optind], flags, writethrough,
+                         force_share, opts)) {
                 exit(1);
             }
         }
-- 
2.9.3

[Qemu-devel] [PATCH v15 08/21] iotests: 030: Prepare for image locking

[Fam Zheng]

qemu-img and qemu-io commands when guest is running need "-U" option,
add it.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 tests/qemu-iotests/030 | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index 0d472d5..e00c11b 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -63,8 +63,8 @@ class TestSingleDrive(iotests.QMPTestCase):
     def test_stream_intermediate(self):
         self.assert_no_active_block_jobs()
 
-        self.assertNotEqual(qemu_io('-f', 'raw', '-c', 'map', backing_img),
-                            qemu_io('-f', iotests.imgfmt, '-c', 'map', mid_img),
+        self.assertNotEqual(qemu_io('-f', 'raw', '-rU', '-c', 'map', backing_img),
+                            qemu_io('-f', iotests.imgfmt, '-rU', '-c', 'map', mid_img),
                             'image file map matches backing file before streaming')
 
         result = self.vm.qmp('block-stream', device='mid', job_id='stream-mid')
@@ -114,7 +114,7 @@ class TestSingleDrive(iotests.QMPTestCase):
         self.assert_no_active_block_jobs()
 
         # The image map is empty before the operation
-        empty_map = qemu_io('-f', iotests.imgfmt, '-c', 'map', test_img)
+        empty_map = qemu_io('-f', iotests.imgfmt, '-rU', '-c', 'map', test_img)
 
         # This is a no-op: no data should ever be copied from the base image
         result = self.vm.qmp('block-stream', device='drive0', base=mid_img)
@@ -197,8 +197,8 @@ class TestParallelOps(iotests.QMPTestCase):
 
         # Check that the maps don't match before the streaming operations
         for i in range(2, self.num_imgs, 2):
-            self.assertNotEqual(qemu_io('-f', iotests.imgfmt, '-c', 'map', self.imgs[i]),
-                                qemu_io('-f', iotests.imgfmt, '-c', 'map', self.imgs[i-1]),
+            self.assertNotEqual(qemu_io('-f', iotests.imgfmt, '-rU', '-c', 'map', self.imgs[i]),
+                                qemu_io('-f', iotests.imgfmt, '-rU', '-c', 'map', self.imgs[i-1]),
                                 'image file map matches backing file before streaming')
 
         # Create all streaming jobs
@@ -351,8 +351,8 @@ class TestParallelOps(iotests.QMPTestCase):
     def test_stream_base_node_name(self):
         self.assert_no_active_block_jobs()
 
-        self.assertNotEqual(qemu_io('-f', iotests.imgfmt, '-c', 'map', self.imgs[4]),
-                            qemu_io('-f', iotests.imgfmt, '-c', 'map', self.imgs[3]),
+        self.assertNotEqual(qemu_io('-f', iotests.imgfmt, '-rU', '-c', 'map', self.imgs[4]),
+                            qemu_io('-f', iotests.imgfmt, '-rU', '-c', 'map', self.imgs[3]),
                             'image file map matches backing file before streaming')
 
         # Error: the base node does not exist
@@ -422,8 +422,8 @@ class TestQuorum(iotests.QMPTestCase):
         if not iotests.supports_quorum():
             return
 
-        self.assertNotEqual(qemu_io('-f', iotests.imgfmt, '-c', 'map', self.children[0]),
-                            qemu_io('-f', iotests.imgfmt, '-c', 'map', self.backing[0]),
+        self.assertNotEqual(qemu_io('-f', iotests.imgfmt, '-rU', '-c', 'map', self.children[0]),
+                            qemu_io('-f', iotests.imgfmt, '-rU', '-c', 'map', self.backing[0]),
                             'image file map matches backing file before streaming')
 
         self.assert_no_active_block_jobs()
-- 
2.9.3

[Qemu-devel] [PATCH v15 09/21] iotests: 046: Prepare for image locking

[Fam Zheng]

The qemu-img info command is executed while VM is running, add -U option
to avoid the image locking error.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 tests/qemu-iotests/046 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/046 b/tests/qemu-iotests/046
index e528b67..f2ebecf 100755
--- a/tests/qemu-iotests/046
+++ b/tests/qemu-iotests/046
@@ -192,7 +192,7 @@ echo "== Verify image content =="
 
 function verify_io()
 {
-    if ($QEMU_IMG info -f "$IMGFMT" "$TEST_IMG" | grep "compat: 0.10" > /dev/null); then
+    if ($QEMU_IMG info -U -f "$IMGFMT" "$TEST_IMG" | grep "compat: 0.10" > /dev/null); then
         # For v2 images, discarded clusters are read from the backing file
         # Keep the variable empty so that the backing file value can be used as
         # the default below
-- 
2.9.3

[Qemu-devel] [PATCH v15 10/21] iotests: 055: Don't attach the target image already for drive-backup

[Fam Zheng]

Double attach is not a valid usage of the target image, drive-backup
will open the blockdev itself so skip the add_drive call in this case.

Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/055 | 32 ++++++++++++++++++--------------
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/tests/qemu-iotests/055 b/tests/qemu-iotests/055
index aafcd24..ba4da65 100755
--- a/tests/qemu-iotests/055
+++ b/tests/qemu-iotests/055
@@ -458,17 +458,18 @@ class TestDriveCompression(iotests.QMPTestCase):
         except OSError:
             pass
 
-    def do_prepare_drives(self, fmt, args):
+    def do_prepare_drives(self, fmt, args, attach_target):
         self.vm = iotests.VM().add_drive(test_img)
 
         qemu_img('create', '-f', fmt, blockdev_target_img,
                  str(TestDriveCompression.image_len), *args)
-        self.vm.add_drive(blockdev_target_img, format=fmt, interface="none")
+        if attach_target:
+            self.vm.add_drive(blockdev_target_img, format=fmt, interface="none")
 
         self.vm.launch()
 
-    def do_test_compress_complete(self, cmd, format, **args):
-        self.do_prepare_drives(format['type'], format['args'])
+    def do_test_compress_complete(self, cmd, format, attach_target, **args):
+        self.do_prepare_drives(format['type'], format['args'], attach_target)
 
         self.assert_no_active_block_jobs()
 
@@ -484,15 +485,16 @@ class TestDriveCompression(iotests.QMPTestCase):
 
     def test_complete_compress_drive_backup(self):
         for format in TestDriveCompression.fmt_supports_compression:
-            self.do_test_compress_complete('drive-backup', format,
+            self.do_test_compress_complete('drive-backup', format, False,
                                            target=blockdev_target_img, mode='existing')
 
     def test_complete_compress_blockdev_backup(self):
         for format in TestDriveCompression.fmt_supports_compression:
-            self.do_test_compress_complete('blockdev-backup', format, target='drive1')
+            self.do_test_compress_complete('blockdev-backup', format, True,
+                                           target='drive1')
 
-    def do_test_compress_cancel(self, cmd, format, **args):
-        self.do_prepare_drives(format['type'], format['args'])
+    def do_test_compress_cancel(self, cmd, format, attach_target, **args):
+        self.do_prepare_drives(format['type'], format['args'], attach_target)
 
         self.assert_no_active_block_jobs()
 
@@ -506,15 +508,16 @@ class TestDriveCompression(iotests.QMPTestCase):
 
     def test_compress_cancel_drive_backup(self):
         for format in TestDriveCompression.fmt_supports_compression:
-            self.do_test_compress_cancel('drive-backup', format,
+            self.do_test_compress_cancel('drive-backup', format, False,
                                          target=blockdev_target_img, mode='existing')
 
     def test_compress_cancel_blockdev_backup(self):
        for format in TestDriveCompression.fmt_supports_compression:
-            self.do_test_compress_cancel('blockdev-backup', format, target='drive1')
+            self.do_test_compress_cancel('blockdev-backup', format, True,
+                                         target='drive1')
 
-    def do_test_compress_pause(self, cmd, format, **args):
-        self.do_prepare_drives(format['type'], format['args'])
+    def do_test_compress_pause(self, cmd, format, attach_target, **args):
+        self.do_prepare_drives(format['type'], format['args'], attach_target)
 
         self.assert_no_active_block_jobs()
 
@@ -546,12 +549,13 @@ class TestDriveCompression(iotests.QMPTestCase):
 
     def test_compress_pause_drive_backup(self):
         for format in TestDriveCompression.fmt_supports_compression:
-            self.do_test_compress_pause('drive-backup', format,
+            self.do_test_compress_pause('drive-backup', format, False,
                                         target=blockdev_target_img, mode='existing')
 
     def test_compress_pause_blockdev_backup(self):
         for format in TestDriveCompression.fmt_supports_compression:
-            self.do_test_compress_pause('blockdev-backup', format, target='drive1')
+            self.do_test_compress_pause('blockdev-backup', format, True,
+                                        target='drive1')
 
 if __name__ == '__main__':
     iotests.main(supported_fmts=['raw', 'qcow2'])
-- 
2.9.3

[Qemu-devel] [PATCH v15 11/21] iotests: 085: Avoid image locking conflict

[Fam Zheng]

In the case where we test the expected error when a blockdev-snapshot
target already has a backing image, the backing chain is opened multiple
times. This will be a problem when we use image locking, so use a
different backing file that is not already open.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 tests/qemu-iotests/085     | 34 ++++++++++++++++++++--------------
 tests/qemu-iotests/085.out |  3 ++-
 2 files changed, 22 insertions(+), 15 deletions(-)

diff --git a/tests/qemu-iotests/085 b/tests/qemu-iotests/085
index c53e97f..cc6efd8 100755
--- a/tests/qemu-iotests/085
+++ b/tests/qemu-iotests/085
@@ -45,7 +45,7 @@ _cleanup()
         rm -f "${TEST_DIR}/${i}-${snapshot_virt0}"
         rm -f "${TEST_DIR}/${i}-${snapshot_virt1}"
     done
-    rm -f "${TEST_IMG}.1" "${TEST_IMG}.2"
+    rm -f "${TEST_IMG}" "${TEST_IMG}.1" "${TEST_IMG}.2" "${TEST_IMG}.base"
 
 }
 trap "_cleanup; exit \$status" 0 1 2 3 15
@@ -87,24 +87,26 @@ function create_group_snapshot()
 }
 
 # ${1}: unique identifier for the snapshot filename
-# ${2}: true: open backing images; false: don't open them (default)
+# ${2}: extra_params to the blockdev-add command
+# ${3}: filename
+function do_blockdev_add()
+{
+    cmd="{ 'execute': 'blockdev-add', 'arguments':
+           { 'driver': 'qcow2', 'node-name': 'snap_${1}', ${2}
+             'file':
+             { 'driver': 'file', 'filename': '${3}',
+               'node-name': 'file_${1}' } } }"
+    _send_qemu_cmd $h "${cmd}" "return"
+}
+
+# ${1}: unique identifier for the snapshot filename
 function add_snapshot_image()
 {
-    if [ "${2}" = "true" ]; then
-        extra_params=""
-    else
-        extra_params="'backing': '', "
-    fi
     base_image="${TEST_DIR}/$((${1}-1))-${snapshot_virt0}"
     snapshot_file="${TEST_DIR}/${1}-${snapshot_virt0}"
     _make_test_img -b "${base_image}" "$size"
     mv "${TEST_IMG}" "${snapshot_file}"
-    cmd="{ 'execute': 'blockdev-add', 'arguments':
-           { 'driver': 'qcow2', 'node-name': 'snap_${1}', ${extra_params}
-             'file':
-             { 'driver': 'file', 'filename': '${snapshot_file}',
-               'node-name': 'file_${1}' } } }"
-    _send_qemu_cmd $h "${cmd}" "return"
+    do_blockdev_add "$1" "'backing': '', " "${snapshot_file}"
 }
 
 # ${1}: unique identifier for the snapshot filename
@@ -222,7 +224,11 @@ echo === Invalid command - snapshot node has a backing image ===
 echo
 
 SNAPSHOTS=$((${SNAPSHOTS}+1))
-add_snapshot_image ${SNAPSHOTS} true
+
+_make_test_img "$size"
+mv "${TEST_IMG}" "${TEST_IMG}.base"
+_make_test_img -b "${TEST_IMG}.base" "$size"
+do_blockdev_add ${SNAPSHOTS} "" "${TEST_IMG}"
 blockdev_snapshot ${SNAPSHOTS} error
 
 echo
diff --git a/tests/qemu-iotests/085.out b/tests/qemu-iotests/085.out
index 182acb4..65b0f68 100644
--- a/tests/qemu-iotests/085.out
+++ b/tests/qemu-iotests/085.out
@@ -78,7 +78,8 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 backing_file=TEST_DIR/
 
 === Invalid command - snapshot node has a backing image ===
 
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 backing_file=TEST_DIR/12-snapshot-v0.IMGFMT
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 backing_file=TEST_DIR/t.IMGFMT.base
 {"return": {}}
 {"error": {"class": "GenericError", "desc": "The snapshot already has a backing image"}}
 
-- 
2.9.3

Re: [Qemu-devel] [PATCH v15 11/21] iotests: 085: Avoid image locking conflict

[Kevin Wolf]

Am 26.04.2017 um 05:34 hat Fam Zheng geschrieben:
> In the case where we test the expected error when a blockdev-snapshot
> target already has a backing image, the backing chain is opened multiple
> times. This will be a problem when we use image locking, so use a
> different backing file that is not already open.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  tests/qemu-iotests/085     | 34 ++++++++++++++++++++--------------
>  tests/qemu-iotests/085.out |  3 ++-
>  2 files changed, 22 insertions(+), 15 deletions(-)
> 
> diff --git a/tests/qemu-iotests/085 b/tests/qemu-iotests/085
> index c53e97f..cc6efd8 100755
> --- a/tests/qemu-iotests/085
> +++ b/tests/qemu-iotests/085
> @@ -45,7 +45,7 @@ _cleanup()
>          rm -f "${TEST_DIR}/${i}-${snapshot_virt0}"
>          rm -f "${TEST_DIR}/${i}-${snapshot_virt1}"
>      done
> -    rm -f "${TEST_IMG}.1" "${TEST_IMG}.2"
> +    rm -f "${TEST_IMG}" "${TEST_IMG}.1" "${TEST_IMG}.2" "${TEST_IMG}.base"
>  
>  }
>  trap "_cleanup; exit \$status" 0 1 2 3 15
> @@ -87,24 +87,26 @@ function create_group_snapshot()
>  }
>  
>  # ${1}: unique identifier for the snapshot filename
> -# ${2}: true: open backing images; false: don't open them (default)
> +# ${2}: extra_params to the blockdev-add command
> +# ${3}: filename
> +function do_blockdev_add()
> +{
> +    cmd="{ 'execute': 'blockdev-add', 'arguments':
> +           { 'driver': 'qcow2', 'node-name': 'snap_${1}', ${2}
> +             'file':
> +             { 'driver': 'file', 'filename': '${3}',
> +               'node-name': 'file_${1}' } } }"
> +    _send_qemu_cmd $h "${cmd}" "return"
> +}
> +
> +# ${1}: unique identifier for the snapshot filename
>  function add_snapshot_image()
>  {
> -    if [ "${2}" = "true" ]; then
> -        extra_params=""
> -    else
> -        extra_params="'backing': '', "
> -    fi
>      base_image="${TEST_DIR}/$((${1}-1))-${snapshot_virt0}"
>      snapshot_file="${TEST_DIR}/${1}-${snapshot_virt0}"
>      _make_test_img -b "${base_image}" "$size"
>      mv "${TEST_IMG}" "${snapshot_file}"
> -    cmd="{ 'execute': 'blockdev-add', 'arguments':
> -           { 'driver': 'qcow2', 'node-name': 'snap_${1}', ${extra_params}
> -             'file':
> -             { 'driver': 'file', 'filename': '${snapshot_file}',
> -               'node-name': 'file_${1}' } } }"
> -    _send_qemu_cmd $h "${cmd}" "return"
> +    do_blockdev_add "$1" "'backing': '', " "${snapshot_file}"
>  }
>  
>  # ${1}: unique identifier for the snapshot filename
> @@ -222,7 +224,11 @@ echo === Invalid command - snapshot node has a backing image ===
>  echo
>  
>  SNAPSHOTS=$((${SNAPSHOTS}+1))
> -add_snapshot_image ${SNAPSHOTS} true
> +
> +_make_test_img "$size"
> +mv "${TEST_IMG}" "${TEST_IMG}.base"

The common idiom is:

    TEST_IMG="$TEST_IMG.base" _make_test_img "$size"

This avoids using mv, which is helpful if we ever want to extend the
testcase for non-file protocols.

But I see that you just copied this from add_snapshot_image(), so it's
preexisting and matter for a different patch/series.

> +_make_test_img -b "${TEST_IMG}.base" "$size"
> +do_blockdev_add ${SNAPSHOTS} "" "${TEST_IMG}"
>  blockdev_snapshot ${SNAPSHOTS} error

Kevin

Re: [Qemu-devel] [PATCH v15 11/21] iotests: 085: Avoid image locking conflict

[Fam Zheng]

On Wed, 04/26 14:30, Kevin Wolf wrote:
> Am 26.04.2017 um 05:34 hat Fam Zheng geschrieben:
> > In the case where we test the expected error when a blockdev-snapshot
> > target already has a backing image, the backing chain is opened multiple
> > times. This will be a problem when we use image locking, so use a
> > different backing file that is not already open.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  tests/qemu-iotests/085     | 34 ++++++++++++++++++++--------------
> >  tests/qemu-iotests/085.out |  3 ++-
> >  2 files changed, 22 insertions(+), 15 deletions(-)
> > 
> > diff --git a/tests/qemu-iotests/085 b/tests/qemu-iotests/085
> > index c53e97f..cc6efd8 100755
> > --- a/tests/qemu-iotests/085
> > +++ b/tests/qemu-iotests/085
> > @@ -45,7 +45,7 @@ _cleanup()
> >          rm -f "${TEST_DIR}/${i}-${snapshot_virt0}"
> >          rm -f "${TEST_DIR}/${i}-${snapshot_virt1}"
> >      done
> > -    rm -f "${TEST_IMG}.1" "${TEST_IMG}.2"
> > +    rm -f "${TEST_IMG}" "${TEST_IMG}.1" "${TEST_IMG}.2" "${TEST_IMG}.base"
> >  
> >  }
> >  trap "_cleanup; exit \$status" 0 1 2 3 15
> > @@ -87,24 +87,26 @@ function create_group_snapshot()
> >  }
> >  
> >  # ${1}: unique identifier for the snapshot filename
> > -# ${2}: true: open backing images; false: don't open them (default)
> > +# ${2}: extra_params to the blockdev-add command
> > +# ${3}: filename
> > +function do_blockdev_add()
> > +{
> > +    cmd="{ 'execute': 'blockdev-add', 'arguments':
> > +           { 'driver': 'qcow2', 'node-name': 'snap_${1}', ${2}
> > +             'file':
> > +             { 'driver': 'file', 'filename': '${3}',
> > +               'node-name': 'file_${1}' } } }"
> > +    _send_qemu_cmd $h "${cmd}" "return"
> > +}
> > +
> > +# ${1}: unique identifier for the snapshot filename
> >  function add_snapshot_image()
> >  {
> > -    if [ "${2}" = "true" ]; then
> > -        extra_params=""
> > -    else
> > -        extra_params="'backing': '', "
> > -    fi
> >      base_image="${TEST_DIR}/$((${1}-1))-${snapshot_virt0}"
> >      snapshot_file="${TEST_DIR}/${1}-${snapshot_virt0}"
> >      _make_test_img -b "${base_image}" "$size"
> >      mv "${TEST_IMG}" "${snapshot_file}"
> > -    cmd="{ 'execute': 'blockdev-add', 'arguments':
> > -           { 'driver': 'qcow2', 'node-name': 'snap_${1}', ${extra_params}
> > -             'file':
> > -             { 'driver': 'file', 'filename': '${snapshot_file}',
> > -               'node-name': 'file_${1}' } } }"
> > -    _send_qemu_cmd $h "${cmd}" "return"
> > +    do_blockdev_add "$1" "'backing': '', " "${snapshot_file}"
> >  }
> >  
> >  # ${1}: unique identifier for the snapshot filename
> > @@ -222,7 +224,11 @@ echo === Invalid command - snapshot node has a backing image ===
> >  echo
> >  
> >  SNAPSHOTS=$((${SNAPSHOTS}+1))
> > -add_snapshot_image ${SNAPSHOTS} true
> > +
> > +_make_test_img "$size"
> > +mv "${TEST_IMG}" "${TEST_IMG}.base"
> 
> The common idiom is:
> 
>     TEST_IMG="$TEST_IMG.base" _make_test_img "$size"
> 
> This avoids using mv, which is helpful if we ever want to extend the
> testcase for non-file protocols.

Yes. Cleaning up "mv" usages in all iotests should be done, once again.

I can change this one in this patch, though.

Fam

[Qemu-devel] [PATCH v15 12/21] iotests: 087: Don't attach test image twice

[Fam Zheng]

The test scenario doesn't require the same image, instead it focuses on
the duplicated node-name, so use null-co to avoid locking conflict.

Reviewed-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
---
 tests/qemu-iotests/087 | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/087 b/tests/qemu-iotests/087
index 9de57dd..6d52f7d 100755
--- a/tests/qemu-iotests/087
+++ b/tests/qemu-iotests/087
@@ -82,8 +82,7 @@ run_qemu -drive driver=$IMGFMT,id=disk,node-name=test-node,file="$TEST_IMG" <<EO
       "driver": "$IMGFMT",
       "node-name": "disk",
       "file": {
-          "driver": "file",
-          "filename": "$TEST_IMG"
+          "driver": "null-co"
       }
     }
   }
@@ -92,8 +91,7 @@ run_qemu -drive driver=$IMGFMT,id=disk,node-name=test-node,file="$TEST_IMG" <<EO
       "driver": "$IMGFMT",
       "node-name": "test-node",
       "file": {
-          "driver": "file",
-          "filename": "$TEST_IMG"
+          "driver": "null-co"
       }
     }
   }
-- 
2.9.3

[Qemu-devel] [PATCH v15 13/21] iotests: 091: Quit QEMU before checking image

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/091 | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/qemu-iotests/091 b/tests/qemu-iotests/091
index 32bbd56..10ac4a8 100755
--- a/tests/qemu-iotests/091
+++ b/tests/qemu-iotests/091
@@ -95,7 +95,9 @@ echo "vm2: qemu process running successfully"
 echo "vm2: flush io, and quit"
 _send_qemu_cmd $h2 'qemu-io disk flush' "(qemu)"
 _send_qemu_cmd $h2 'quit' ""
+_send_qemu_cmd $h1 'quit' ""
 
+wait
 echo "Check image pattern"
 ${QEMU_IO} -c "read -P 0x22 0 4M" "${TEST_IMG}" | _filter_testdir | _filter_qemu_io
 
-- 
2.9.3

Re: [Qemu-devel] [PATCH v15 13/21] iotests: 091: Quit QEMU before checking image

[Kevin Wolf]

Am 26.04.2017 um 05:34 hat Fam Zheng geschrieben:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/091 | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/tests/qemu-iotests/091 b/tests/qemu-iotests/091
> index 32bbd56..10ac4a8 100755
> --- a/tests/qemu-iotests/091
> +++ b/tests/qemu-iotests/091
> @@ -95,7 +95,9 @@ echo "vm2: qemu process running successfully"
>  echo "vm2: flush io, and quit"
>  _send_qemu_cmd $h2 'qemu-io disk flush' "(qemu)"
>  _send_qemu_cmd $h2 'quit' ""
> +_send_qemu_cmd $h1 'quit' ""
>  
> +wait

I think it's better to use the function from common.qemu for this:

    wait=1 _cleanup_qemu

Kevin

Re: [Qemu-devel] [PATCH v15 13/21] iotests: 091: Quit QEMU before checking image

[Fam Zheng]

On Wed, 04/26 14:34, Kevin Wolf wrote:
> Am 26.04.2017 um 05:34 hat Fam Zheng geschrieben:
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > Reviewed-by: Max Reitz <mreitz@redhat.com>
> > ---
> >  tests/qemu-iotests/091 | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/tests/qemu-iotests/091 b/tests/qemu-iotests/091
> > index 32bbd56..10ac4a8 100755
> > --- a/tests/qemu-iotests/091
> > +++ b/tests/qemu-iotests/091
> > @@ -95,7 +95,9 @@ echo "vm2: qemu process running successfully"
> >  echo "vm2: flush io, and quit"
> >  _send_qemu_cmd $h2 'qemu-io disk flush' "(qemu)"
> >  _send_qemu_cmd $h2 'quit' ""
> > +_send_qemu_cmd $h1 'quit' ""
> >  
> > +wait
> 
> I think it's better to use the function from common.qemu for this:
> 
>     wait=1 _cleanup_qemu
> 

I didn't look into it, but that would get such an error:

091 1s ... - output mismatch (see 091.out.bad)
--- /stor/work/qemu/tests/qemu-iotests/091.out	2016-10-27 14:31:53.671300277 +0800
+++ 091.out.bad	2017-04-27 15:03:28.965504185 +0800
@@ -18,6 +18,20 @@
 vm2: qemu-io disk write complete
 vm2: qemu process running successfully
 vm2: flush io, and quit
+total time: 48 milliseconds
+downtime: 25 milliseconds
+setup: 1 milliseconds
+transferred ram: 465 kbytes
+throughput: 99.28 mbps
+remaining ram: 0 kbytes
+total ram: 131592 kbytes
+duplicate: 32854 pages
+skipped: 0 pages
+normal: 44 pages
+normal bytes: 176 kbytes
+dirty sync count: 2
+(qemu) quit
+(qemu) quit
 Check image pattern
 read 4194304/4194304 bytes at offset 0
 4 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)

[Qemu-devel] [PATCH v15 14/21] iotests: 172: Use separate images for multiple devices

[Fam Zheng]

To avoid image lock failures.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 tests/qemu-iotests/172     | 55 +++++++++++++++++++++++++---------------------
 tests/qemu-iotests/172.out | 50 +++++++++++++++++++++--------------------
 2 files changed, 56 insertions(+), 49 deletions(-)

diff --git a/tests/qemu-iotests/172 b/tests/qemu-iotests/172
index 1b7d3a1..826d6fe 100755
--- a/tests/qemu-iotests/172
+++ b/tests/qemu-iotests/172
@@ -30,6 +30,8 @@ status=1	# failure is the default!
 _cleanup()
 {
 	_cleanup_test_img
+    rm -f "$TEST_IMG.2"
+    rm -f "$TEST_IMG.3"
 }
 trap "_cleanup; exit \$status" 0 1 2 3 15
 
@@ -86,6 +88,9 @@ size=720k
 
 _make_test_img $size
 
+TEST_IMG="$TEST_IMG.2" _make_test_img $size
+TEST_IMG="$TEST_IMG.3" _make_test_img $size
+
 # Default drive semantics:
 #
 # By default you get a single empty floppy drive. You can override it with
@@ -105,7 +110,7 @@ echo === Using -fda/-fdb options ===
 
 check_floppy_qtree -fda "$TEST_IMG"
 check_floppy_qtree -fdb "$TEST_IMG"
-check_floppy_qtree -fda "$TEST_IMG" -fdb "$TEST_IMG"
+check_floppy_qtree -fda "$TEST_IMG" -fdb "$TEST_IMG.2"
 
 
 echo
@@ -114,7 +119,7 @@ echo === Using -drive options ===
 
 check_floppy_qtree -drive if=floppy,file="$TEST_IMG"
 check_floppy_qtree -drive if=floppy,file="$TEST_IMG",index=1
-check_floppy_qtree -drive if=floppy,file="$TEST_IMG" -drive if=floppy,file="$TEST_IMG",index=1
+check_floppy_qtree -drive if=floppy,file="$TEST_IMG" -drive if=floppy,file="$TEST_IMG.2",index=1
 
 echo
 echo
@@ -122,7 +127,7 @@ echo === Using -drive if=none and -global ===
 
 check_floppy_qtree -drive if=none,file="$TEST_IMG" -global isa-fdc.driveA=none0
 check_floppy_qtree -drive if=none,file="$TEST_IMG" -global isa-fdc.driveB=none0
-check_floppy_qtree -drive if=none,file="$TEST_IMG" -drive if=none,file="$TEST_IMG" \
+check_floppy_qtree -drive if=none,file="$TEST_IMG" -drive if=none,file="$TEST_IMG.2" \
                    -global isa-fdc.driveA=none0 -global isa-fdc.driveB=none1
 
 echo
@@ -131,7 +136,7 @@ echo === Using -drive if=none and -device ===
 
 check_floppy_qtree -drive if=none,file="$TEST_IMG" -device floppy,drive=none0
 check_floppy_qtree -drive if=none,file="$TEST_IMG" -device floppy,drive=none0,unit=1
-check_floppy_qtree -drive if=none,file="$TEST_IMG" -drive if=none,file="$TEST_IMG" \
+check_floppy_qtree -drive if=none,file="$TEST_IMG" -drive if=none,file="$TEST_IMG.2" \
                    -device floppy,drive=none0 -device floppy,drive=none1,unit=1
 
 echo
@@ -139,58 +144,58 @@ echo
 echo === Mixing -fdX and -global ===
 
 # Working
-check_floppy_qtree -fda "$TEST_IMG" -drive if=none,file="$TEST_IMG" -global isa-fdc.driveB=none0
-check_floppy_qtree -fdb "$TEST_IMG" -drive if=none,file="$TEST_IMG" -global isa-fdc.driveA=none0
+check_floppy_qtree -fda "$TEST_IMG" -drive if=none,file="$TEST_IMG.2" -global isa-fdc.driveB=none0
+check_floppy_qtree -fdb "$TEST_IMG" -drive if=none,file="$TEST_IMG.2" -global isa-fdc.driveA=none0
 
 # Conflicting (-fdX wins)
-check_floppy_qtree -fda "$TEST_IMG" -drive if=none,file="$TEST_IMG" -global isa-fdc.driveA=none0
-check_floppy_qtree -fdb "$TEST_IMG" -drive if=none,file="$TEST_IMG" -global isa-fdc.driveB=none0
+check_floppy_qtree -fda "$TEST_IMG" -drive if=none,file="$TEST_IMG.2" -global isa-fdc.driveA=none0
+check_floppy_qtree -fdb "$TEST_IMG" -drive if=none,file="$TEST_IMG.2" -global isa-fdc.driveB=none0
 
 echo
 echo
 echo === Mixing -fdX and -device ===
 
 # Working
-check_floppy_qtree -fda "$TEST_IMG" -drive if=none,file="$TEST_IMG" -device floppy,drive=none0
-check_floppy_qtree -fda "$TEST_IMG" -drive if=none,file="$TEST_IMG" -device floppy,drive=none0,unit=1
+check_floppy_qtree -fda "$TEST_IMG" -drive if=none,file="$TEST_IMG.2" -device floppy,drive=none0
+check_floppy_qtree -fda "$TEST_IMG" -drive if=none,file="$TEST_IMG.2" -device floppy,drive=none0,unit=1
 
-check_floppy_qtree -fdb "$TEST_IMG" -drive if=none,file="$TEST_IMG" -device floppy,drive=none0
-check_floppy_qtree -fdb "$TEST_IMG" -drive if=none,file="$TEST_IMG" -device floppy,drive=none0,unit=0
+check_floppy_qtree -fdb "$TEST_IMG" -drive if=none,file="$TEST_IMG.2" -device floppy,drive=none0
+check_floppy_qtree -fdb "$TEST_IMG" -drive if=none,file="$TEST_IMG.2" -device floppy,drive=none0,unit=0
 
 # Conflicting
-check_floppy_qtree -fda "$TEST_IMG" -drive if=none,file="$TEST_IMG" -device floppy,drive=none0,unit=0
-check_floppy_qtree -fdb "$TEST_IMG" -drive if=none,file="$TEST_IMG" -device floppy,drive=none0,unit=1
+check_floppy_qtree -fda "$TEST_IMG" -drive if=none,file="$TEST_IMG.2" -device floppy,drive=none0,unit=0
+check_floppy_qtree -fdb "$TEST_IMG" -drive if=none,file="$TEST_IMG.2" -device floppy,drive=none0,unit=1
 
 echo
 echo
 echo === Mixing -drive and -device ===
 
 # Working
-check_floppy_qtree -drive if=floppy,file="$TEST_IMG" -drive if=none,file="$TEST_IMG" -device floppy,drive=none0
-check_floppy_qtree -drive if=floppy,file="$TEST_IMG" -drive if=none,file="$TEST_IMG" -device floppy,drive=none0,unit=1
+check_floppy_qtree -drive if=floppy,file="$TEST_IMG" -drive if=none,file="$TEST_IMG.2" -device floppy,drive=none0
+check_floppy_qtree -drive if=floppy,file="$TEST_IMG" -drive if=none,file="$TEST_IMG.2" -device floppy,drive=none0,unit=1
 
 # Conflicting
-check_floppy_qtree -drive if=floppy,file="$TEST_IMG" -drive if=none,file="$TEST_IMG" -device floppy,drive=none0,unit=0
+check_floppy_qtree -drive if=floppy,file="$TEST_IMG" -drive if=none,file="$TEST_IMG.2" -device floppy,drive=none0,unit=0
 
 echo
 echo
 echo === Mixing -global and -device ===
 
 # Working
-check_floppy_qtree -drive if=none,file="$TEST_IMG" -drive if=none,file="$TEST_IMG" \
+check_floppy_qtree -drive if=none,file="$TEST_IMG" -drive if=none,file="$TEST_IMG.2" \
                    -global isa-fdc.driveA=none0 -device floppy,drive=none1
-check_floppy_qtree -drive if=none,file="$TEST_IMG" -drive if=none,file="$TEST_IMG" \
+check_floppy_qtree -drive if=none,file="$TEST_IMG" -drive if=none,file="$TEST_IMG.2" \
                    -global isa-fdc.driveA=none0 -device floppy,drive=none1,unit=1
 
-check_floppy_qtree -drive if=none,file="$TEST_IMG" -drive if=none,file="$TEST_IMG" \
+check_floppy_qtree -drive if=none,file="$TEST_IMG" -drive if=none,file="$TEST_IMG.2" \
                    -global isa-fdc.driveB=none0 -device floppy,drive=none1
-check_floppy_qtree -drive if=none,file="$TEST_IMG" -drive if=none,file="$TEST_IMG" \
+check_floppy_qtree -drive if=none,file="$TEST_IMG" -drive if=none,file="$TEST_IMG.2" \
                    -global isa-fdc.driveB=none0 -device floppy,drive=none1,unit=0
 
 # Conflicting
-check_floppy_qtree -drive if=none,file="$TEST_IMG" -drive if=none,file="$TEST_IMG" \
+check_floppy_qtree -drive if=none,file="$TEST_IMG" -drive if=none,file="$TEST_IMG.2" \
                    -global isa-fdc.driveA=none0 -device floppy,drive=none1,unit=0
-check_floppy_qtree -drive if=none,file="$TEST_IMG" -drive if=none,file="$TEST_IMG" \
+check_floppy_qtree -drive if=none,file="$TEST_IMG" -drive if=none,file="$TEST_IMG.2" \
                    -global isa-fdc.driveB=none0 -device floppy,drive=none1,unit=1
 
 echo
@@ -199,8 +204,8 @@ echo === Too many floppy drives ===
 
 # Working
 check_floppy_qtree -drive if=floppy,file="$TEST_IMG" \
-                   -drive if=none,file="$TEST_IMG" \
-                   -drive if=none,file="$TEST_IMG" \
+                   -drive if=none,file="$TEST_IMG.2" \
+                   -drive if=none,file="$TEST_IMG.3" \
                    -global isa-fdc.driveB=none0 \
                    -device floppy,drive=none1
 
diff --git a/tests/qemu-iotests/172.out b/tests/qemu-iotests/172.out
index 54b5329..2732966 100644
--- a/tests/qemu-iotests/172.out
+++ b/tests/qemu-iotests/172.out
@@ -1,5 +1,7 @@
 QA output created by 172
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=737280
+Formatting 'TEST_DIR/t.IMGFMT.2', fmt=IMGFMT size=737280
+Formatting 'TEST_DIR/t.IMGFMT.3', fmt=IMGFMT size=737280
 
 
 === Default ===
@@ -99,7 +101,7 @@ Testing: -fdb TEST_DIR/t.qcow2
                 share-rw = false
                 drive-type = "288"
 
-Testing: -fda TEST_DIR/t.qcow2 -fdb TEST_DIR/t.qcow2
+Testing: -fda TEST_DIR/t.qcow2 -fdb TEST_DIR/t.qcow2.2
 
           dev: isa-fdc, id ""
             iobase = 1008 (0x3f0)
@@ -205,7 +207,7 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2,index=1
                 share-rw = false
                 drive-type = "288"
 
-Testing: -drive if=floppy,file=TEST_DIR/t.qcow2 -drive if=floppy,file=TEST_DIR/t.qcow2,index=1
+Testing: -drive if=floppy,file=TEST_DIR/t.qcow2 -drive if=floppy,file=TEST_DIR/t.qcow2.2,index=1
 
           dev: isa-fdc, id ""
             iobase = 1008 (0x3f0)
@@ -300,7 +302,7 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -global isa-fdc.driveB=none0
                 share-rw = false
                 drive-type = "144"
 
-Testing: -drive if=none,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2 -global isa-fdc.driveA=none0 -global isa-fdc.driveB=none1
+Testing: -drive if=none,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -global isa-fdc.driveA=none0 -global isa-fdc.driveB=none1
 
           dev: isa-fdc, id ""
             iobase = 1008 (0x3f0)
@@ -395,7 +397,7 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0,unit=1
                 share-rw = false
                 drive-type = "144"
 
-Testing: -drive if=none,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0 -device floppy,drive=none1,unit=1
+Testing: -drive if=none,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device floppy,drive=none0 -device floppy,drive=none1,unit=1
 
           dev: isa-fdc, id ""
             iobase = 1008 (0x3f0)
@@ -436,7 +438,7 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qco
 
 === Mixing -fdX and -global ===
 
-Testing: -fda TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2 -global isa-fdc.driveB=none0
+Testing: -fda TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -global isa-fdc.driveB=none0
 
           dev: isa-fdc, id ""
             iobase = 1008 (0x3f0)
@@ -474,7 +476,7 @@ Testing: -fda TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2 -global isa-
                 share-rw = false
                 drive-type = "144"
 
-Testing: -fdb TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2 -global isa-fdc.driveA=none0
+Testing: -fdb TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -global isa-fdc.driveA=none0
 
           dev: isa-fdc, id ""
             iobase = 1008 (0x3f0)
@@ -512,7 +514,7 @@ Testing: -fdb TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2 -global isa-
                 share-rw = false
                 drive-type = "144"
 
-Testing: -fda TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2 -global isa-fdc.driveA=none0
+Testing: -fda TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -global isa-fdc.driveA=none0
 
           dev: isa-fdc, id ""
             iobase = 1008 (0x3f0)
@@ -539,7 +541,7 @@ Testing: -fda TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2 -global isa-
                 share-rw = false
                 drive-type = "144"
 
-Testing: -fdb TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2 -global isa-fdc.driveB=none0
+Testing: -fdb TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -global isa-fdc.driveB=none0
 
           dev: isa-fdc, id ""
             iobase = 1008 (0x3f0)
@@ -569,7 +571,7 @@ Testing: -fdb TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2 -global isa-
 
 === Mixing -fdX and -device ===
 
-Testing: -fda TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0
+Testing: -fda TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device floppy,drive=none0
 
           dev: isa-fdc, id ""
             iobase = 1008 (0x3f0)
@@ -607,7 +609,7 @@ Testing: -fda TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2 -device flop
                 share-rw = false
                 drive-type = "144"
 
-Testing: -fda TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0,unit=1
+Testing: -fda TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device floppy,drive=none0,unit=1
 
           dev: isa-fdc, id ""
             iobase = 1008 (0x3f0)
@@ -645,7 +647,7 @@ Testing: -fda TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2 -device flop
                 share-rw = false
                 drive-type = "144"
 
-Testing: -fdb TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0
+Testing: -fdb TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device floppy,drive=none0
 
           dev: isa-fdc, id ""
             iobase = 1008 (0x3f0)
@@ -683,7 +685,7 @@ Testing: -fdb TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2 -device flop
                 share-rw = false
                 drive-type = "144"
 
-Testing: -fdb TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0,unit=0
+Testing: -fdb TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device floppy,drive=none0,unit=0
 
           dev: isa-fdc, id ""
             iobase = 1008 (0x3f0)
@@ -721,18 +723,18 @@ Testing: -fdb TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2 -device flop
                 share-rw = false
                 drive-type = "144"
 
-Testing: -fda TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0,unit=0
+Testing: -fda TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device floppy,drive=none0,unit=0
 QEMU_PROG: -device floppy,drive=none0,unit=0: Floppy unit 0 is in use
 QEMU_PROG: -device floppy,drive=none0,unit=0: Device initialization failed.
 
-Testing: -fdb TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0,unit=1
+Testing: -fdb TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device floppy,drive=none0,unit=1
 QEMU_PROG: -device floppy,drive=none0,unit=1: Floppy unit 1 is in use
 QEMU_PROG: -device floppy,drive=none0,unit=1: Device initialization failed.
 
 
 === Mixing -drive and -device ===
 
-Testing: -drive if=floppy,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0
+Testing: -drive if=floppy,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device floppy,drive=none0
 
           dev: isa-fdc, id ""
             iobase = 1008 (0x3f0)
@@ -770,7 +772,7 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.q
                 share-rw = false
                 drive-type = "144"
 
-Testing: -drive if=floppy,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0,unit=1
+Testing: -drive if=floppy,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device floppy,drive=none0,unit=1
 
           dev: isa-fdc, id ""
             iobase = 1008 (0x3f0)
@@ -808,14 +810,14 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.q
                 share-rw = false
                 drive-type = "144"
 
-Testing: -drive if=floppy,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0,unit=0
+Testing: -drive if=floppy,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device floppy,drive=none0,unit=0
 QEMU_PROG: -device floppy,drive=none0,unit=0: Floppy unit 0 is in use
 QEMU_PROG: -device floppy,drive=none0,unit=0: Device initialization failed.
 
 
 === Mixing -global and -device ===
 
-Testing: -drive if=none,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2 -global isa-fdc.driveA=none0 -device floppy,drive=none1
+Testing: -drive if=none,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -global isa-fdc.driveA=none0 -device floppy,drive=none1
 
           dev: isa-fdc, id ""
             iobase = 1008 (0x3f0)
@@ -853,7 +855,7 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qco
                 share-rw = false
                 drive-type = "144"
 
-Testing: -drive if=none,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2 -global isa-fdc.driveA=none0 -device floppy,drive=none1,unit=1
+Testing: -drive if=none,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -global isa-fdc.driveA=none0 -device floppy,drive=none1,unit=1
 
           dev: isa-fdc, id ""
             iobase = 1008 (0x3f0)
@@ -891,7 +893,7 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qco
                 share-rw = false
                 drive-type = "144"
 
-Testing: -drive if=none,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2 -global isa-fdc.driveB=none0 -device floppy,drive=none1
+Testing: -drive if=none,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -global isa-fdc.driveB=none0 -device floppy,drive=none1
 
           dev: isa-fdc, id ""
             iobase = 1008 (0x3f0)
@@ -929,7 +931,7 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qco
                 share-rw = false
                 drive-type = "144"
 
-Testing: -drive if=none,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2 -global isa-fdc.driveB=none0 -device floppy,drive=none1,unit=0
+Testing: -drive if=none,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -global isa-fdc.driveB=none0 -device floppy,drive=none1,unit=0
 
           dev: isa-fdc, id ""
             iobase = 1008 (0x3f0)
@@ -967,18 +969,18 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qco
                 share-rw = false
                 drive-type = "144"
 
-Testing: -drive if=none,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2 -global isa-fdc.driveA=none0 -device floppy,drive=none1,unit=0
+Testing: -drive if=none,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -global isa-fdc.driveA=none0 -device floppy,drive=none1,unit=0
 QEMU_PROG: -device floppy,drive=none1,unit=0: Floppy unit 0 is in use
 QEMU_PROG: -device floppy,drive=none1,unit=0: Device initialization failed.
 
-Testing: -drive if=none,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2 -global isa-fdc.driveB=none0 -device floppy,drive=none1,unit=1
+Testing: -drive if=none,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -global isa-fdc.driveB=none0 -device floppy,drive=none1,unit=1
 QEMU_PROG: -device floppy,drive=none1,unit=1: Floppy unit 1 is in use
 QEMU_PROG: -device floppy,drive=none1,unit=1: Device initialization failed.
 
 
 === Too many floppy drives ===
 
-Testing: -drive if=floppy,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2 -global isa-fdc.driveB=none0 -device floppy,drive=none1
+Testing: -drive if=floppy,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -drive if=none,file=TEST_DIR/t.qcow2.3 -global isa-fdc.driveB=none0 -device floppy,drive=none1
 QEMU_PROG: -device floppy,drive=none1: Can't create floppy unit 2, bus supports only 2 units
 QEMU_PROG: -device floppy,drive=none1: Device initialization failed.
 
-- 
2.9.3

[Qemu-devel] [PATCH v15 15/21] tests: Use null-co:// instead of /dev/null as the dummy image

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 tests/drive_del-test.c    | 2 +-
 tests/nvme-test.c         | 2 +-
 tests/usb-hcd-uhci-test.c | 2 +-
 tests/usb-hcd-xhci-test.c | 2 +-
 tests/virtio-blk-test.c   | 2 +-
 tests/virtio-scsi-test.c  | 5 +++--
 6 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/tests/drive_del-test.c b/tests/drive_del-test.c
index 121b9c9..2175139 100644
--- a/tests/drive_del-test.c
+++ b/tests/drive_del-test.c
@@ -92,7 +92,7 @@ static void test_after_failed_device_add(void)
 static void test_drive_del_device_del(void)
 {
     /* Start with a drive used by a device that unplugs instantaneously */
-    qtest_start("-drive if=none,id=drive0,file=/dev/null,format=raw"
+    qtest_start("-drive if=none,id=drive0,file=null-co://,format=raw"
                 " -device virtio-scsi-pci"
                 " -device scsi-hd,drive=drive0,id=dev0");
 
diff --git a/tests/nvme-test.c b/tests/nvme-test.c
index c8bece4..7674a44 100644
--- a/tests/nvme-test.c
+++ b/tests/nvme-test.c
@@ -22,7 +22,7 @@ int main(int argc, char **argv)
     g_test_init(&argc, &argv, NULL);
     qtest_add_func("/nvme/nop", nop);
 
-    qtest_start("-drive id=drv0,if=none,file=/dev/null,format=raw "
+    qtest_start("-drive id=drv0,if=none,file=null-co://,format=raw "
                 "-device nvme,drive=drv0,serial=foo");
     ret = g_test_run();
 
diff --git a/tests/usb-hcd-uhci-test.c b/tests/usb-hcd-uhci-test.c
index f25bae5..5b500fe 100644
--- a/tests/usb-hcd-uhci-test.c
+++ b/tests/usb-hcd-uhci-test.c
@@ -79,7 +79,7 @@ int main(int argc, char **argv)
 {
     const char *arch = qtest_get_arch();
     const char *cmd = "-device piix3-usb-uhci,id=uhci,addr=1d.0"
-                      " -drive id=drive0,if=none,file=/dev/null,format=raw"
+                      " -drive id=drive0,if=none,file=null-co://,format=raw"
                       " -device usb-tablet,bus=uhci.0,port=1";
     int ret;
 
diff --git a/tests/usb-hcd-xhci-test.c b/tests/usb-hcd-xhci-test.c
index 22513e9..031764d 100644
--- a/tests/usb-hcd-xhci-test.c
+++ b/tests/usb-hcd-xhci-test.c
@@ -89,7 +89,7 @@ int main(int argc, char **argv)
     qtest_add_func("/xhci/pci/hotplug/usb-uas", test_usb_uas_hotplug);
 
     qtest_start("-device nec-usb-xhci,id=xhci"
-                " -drive id=drive0,if=none,file=/dev/null,format=raw");
+                " -drive id=drive0,if=none,file=null-co://,format=raw");
     ret = g_test_run();
     qtest_end();
 
diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c
index 1eee95d..fd2078c 100644
--- a/tests/virtio-blk-test.c
+++ b/tests/virtio-blk-test.c
@@ -63,7 +63,7 @@ static QOSState *pci_test_start(void)
     const char *arch = qtest_get_arch();
     char *tmp_path;
     const char *cmd = "-drive if=none,id=drive0,file=%s,format=raw "
-                      "-drive if=none,id=drive1,file=/dev/null,format=raw "
+                      "-drive if=none,id=drive1,file=null-co://,format=raw "
                       "-device virtio-blk-pci,id=drv0,drive=drive0,"
                       "addr=%x.%x";
 
diff --git a/tests/virtio-scsi-test.c b/tests/virtio-scsi-test.c
index 0eabd56..8b0f77a 100644
--- a/tests/virtio-scsi-test.c
+++ b/tests/virtio-scsi-test.c
@@ -35,7 +35,7 @@ typedef struct {
 static QOSState *qvirtio_scsi_start(const char *extra_opts)
 {
     const char *arch = qtest_get_arch();
-    const char *cmd = "-drive id=drv0,if=none,file=/dev/null,format=raw "
+    const char *cmd = "-drive id=drv0,if=none,file=null-co://,format=raw "
                       "-device virtio-scsi-pci,id=vs0 "
                       "-device scsi-hd,bus=vs0.0,drive=drv0 %s";
 
@@ -195,7 +195,8 @@ static void hotplug(void)
     QDict *response;
     QOSState *qs;
 
-    qs = qvirtio_scsi_start("-drive id=drv1,if=none,file=/dev/null,format=raw");
+    qs = qvirtio_scsi_start(
+            "-drive id=drv1,if=none,file=null-co://,format=raw");
     response = qmp("{\"execute\": \"device_add\","
                    " \"arguments\": {"
                    "   \"driver\": \"scsi-hd\","
-- 
2.9.3

[Qemu-devel] [PATCH v15 16/21] file-posix: Add 'locking' option

[Fam Zheng]

Making this option available even before implementing it will let
converting tests easier: in coming patches they can specify the option
already when necessary, before we actually write code to lock the
images.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/file-posix.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/block/file-posix.c b/block/file-posix.c
index ade71db..2114720 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -392,6 +392,11 @@ static QemuOptsList raw_runtime_opts = {
             .type = QEMU_OPT_STRING,
             .help = "host AIO implementation (threads, native)",
         },
+        {
+            .name = "locking",
+            .type = QEMU_OPT_BOOL,
+            .help = "lock the file",
+        },
         { /* end of list */ }
     },
 };
-- 
2.9.3

Re: [Qemu-devel] [PATCH v15 16/21] file-posix: Add 'locking' option

[Kevin Wolf]

Am 26.04.2017 um 05:34 hat Fam Zheng geschrieben:
> Making this option available even before implementing it will let
> converting tests easier: in coming patches they can specify the option
> already when necessary, before we actually write code to lock the
> images.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/file-posix.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index ade71db..2114720 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -392,6 +392,11 @@ static QemuOptsList raw_runtime_opts = {
>              .type = QEMU_OPT_STRING,
>              .help = "host AIO implementation (threads, native)",
>          },
> +        {
> +            .name = "locking",
> +            .type = QEMU_OPT_BOOL,
> +            .help = "lock the file",
> +        },
>          { /* end of list */ }
>      },
>  };

I don't mind whether it happens in this patch or when the option is
actually implemented, but this option needs to be added to the QAPI
schema.

Kevin

Re: [Qemu-devel] [PATCH v15 16/21] file-posix: Add 'locking' option

[Fam Zheng]

On Wed, 04/26 14:41, Kevin Wolf wrote:
> Am 26.04.2017 um 05:34 hat Fam Zheng geschrieben:
> > Making this option available even before implementing it will let
> > converting tests easier: in coming patches they can specify the option
> > already when necessary, before we actually write code to lock the
> > images.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  block/file-posix.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/block/file-posix.c b/block/file-posix.c
> > index ade71db..2114720 100644
> > --- a/block/file-posix.c
> > +++ b/block/file-posix.c
> > @@ -392,6 +392,11 @@ static QemuOptsList raw_runtime_opts = {
> >              .type = QEMU_OPT_STRING,
> >              .help = "host AIO implementation (threads, native)",
> >          },
> > +        {
> > +            .name = "locking",
> > +            .type = QEMU_OPT_BOOL,
> > +            .help = "lock the file",
> > +        },
> >          { /* end of list */ }
> >      },
> >  };
> 
> I don't mind whether it happens in this patch or when the option is
> actually implemented, but this option needs to be added to the QAPI
> schema.

I will add it.

Fam

[Qemu-devel] [PATCH v15 17/21] tests: Disable image lock in test-replication

[Fam Zheng]

The COLO block replication architecture requires one disk to be shared
between primary and secondary, in the test both processes use posix file
protocol (instead of over NBD) so it is affected by image locking.
Disable the lock.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 tests/test-replication.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/tests/test-replication.c b/tests/test-replication.c
index fac2da3..e1c4235 100644
--- a/tests/test-replication.c
+++ b/tests/test-replication.c
@@ -179,7 +179,8 @@ static BlockBackend *start_primary(void)
     char *cmdline;
 
     cmdline = g_strdup_printf("driver=replication,mode=primary,node-name=xxx,"
-                              "file.driver=qcow2,file.file.filename=%s"
+                              "file.driver=qcow2,file.file.filename=%s,"
+                              "file.file.locking=off"
                               , p_local_disk);
     opts = qemu_opts_parse_noisily(&qemu_drive_opts, cmdline, false);
     g_free(cmdline);
@@ -310,7 +311,9 @@ static BlockBackend *start_secondary(void)
     Error *local_err = NULL;
 
     /* add s_local_disk and forge S_LOCAL_DISK_ID */
-    cmdline = g_strdup_printf("file.filename=%s,driver=qcow2", s_local_disk);
+    cmdline = g_strdup_printf("file.filename=%s,driver=qcow2,"
+                              "file.locking=off",
+                              s_local_disk);
     opts = qemu_opts_parse_noisily(&qemu_drive_opts, cmdline, false);
     g_free(cmdline);
 
@@ -331,8 +334,10 @@ static BlockBackend *start_secondary(void)
     /* add S_(ACTIVE/HIDDEN)_DISK and forge S_ID */
     cmdline = g_strdup_printf("driver=replication,mode=secondary,top-id=%s,"
                               "file.driver=qcow2,file.file.filename=%s,"
+                              "file.file.locking=off,"
                               "file.backing.driver=qcow2,"
                               "file.backing.file.filename=%s,"
+                              "file.backing.file.locking=off,"
                               "file.backing.backing=%s"
                               , S_ID, s_active_disk, s_hidden_disk
                               , S_LOCAL_DISK_ID);
-- 
2.9.3

[Qemu-devel] [PATCH v15 18/21] block: Reuse bs as backing hd for drive-backup sync=none

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 blockdev.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/blockdev.c b/blockdev.c
index 4927914..4e04dec 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3174,6 +3174,7 @@ static BlockJob *do_drive_backup(DriveBackup *backup, BlockJobTxn *txn,
     Error *local_err = NULL;
     int flags;
     int64_t size;
+    bool set_backing_hd = false;
 
     if (!backup->has_speed) {
         backup->speed = 0;
@@ -3224,6 +3225,8 @@ static BlockJob *do_drive_backup(DriveBackup *backup, BlockJobTxn *txn,
     }
     if (backup->sync == MIRROR_SYNC_MODE_NONE) {
         source = bs;
+        flags |= BDRV_O_NO_BACKING;
+        set_backing_hd = true;
     }
 
     size = bdrv_getlength(bs);
@@ -3250,7 +3253,9 @@ static BlockJob *do_drive_backup(DriveBackup *backup, BlockJobTxn *txn,
     }
 
     if (backup->format) {
-        options = qdict_new();
+        if (!options) {
+            options = qdict_new();
+        }
         qdict_put(options, "driver", qstring_from_str(backup->format));
     }
 
@@ -3261,6 +3266,14 @@ static BlockJob *do_drive_backup(DriveBackup *backup, BlockJobTxn *txn,
 
     bdrv_set_aio_context(target_bs, aio_context);
 
+    if (set_backing_hd) {
+        bdrv_set_backing_hd(target_bs, source, &local_err);
+        if (local_err) {
+            bdrv_unref(target_bs);
+            goto out;
+        }
+    }
+
     if (backup->has_bitmap) {
         bmap = bdrv_find_dirty_bitmap(bs, backup->bitmap);
         if (!bmap) {
-- 
2.9.3

Re: [Qemu-devel] [PATCH v15 18/21] block: Reuse bs as backing hd for drive-backup sync=none

[Kevin Wolf]

Am 26.04.2017 um 05:34 hat Fam Zheng geschrieben:
> Signed-off-by: Fam Zheng <famz@redhat.com>

The commit message is a bit terse. :-)

So I think this means that instead of opening the backing file of the
backup (which is probably the active file of the VM) a second time, we
instead take a reference on the existing node. Very good idea.

In fact, my question is: How did this ever work? Did we just neglect to
test this? The backup backing file will use stale qcow2 metadata when we
continue to write to the source.

Unless I'm missing something, I'd propose this for qemu-stable.

Also, mirror with MIRROR_OPEN_BACKING_CHAIN probably has a similar
problem with opening the images in the backing chain a second time.

Kevin

Re: [Qemu-devel] [PATCH v15 18/21] block: Reuse bs as backing hd for drive-backup sync=none

[Fam Zheng]

On Wed, 04/26 14:52, Kevin Wolf wrote:
> Am 26.04.2017 um 05:34 hat Fam Zheng geschrieben:
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> 
> The commit message is a bit terse. :-)
> 
> So I think this means that instead of opening the backing file of the
> backup (which is probably the active file of the VM) a second time, we
> instead take a reference on the existing node. Very good idea.
> 
> In fact, my question is: How did this ever work? Did we just neglect to
> test this? The backup backing file will use stale qcow2 metadata when we
> continue to write to the source.

Reading from the target BDS would be a problem, but I guess it's relatively
uncommon:

- In QEMU code, COW is always done manually in backup_do_cow, so no reading from
  target->backing in block layer.

- Writing to target is done in target cluster granularity so no COW too.

But it's still a fully valid point, for example it can be exported to NBD, etc.

> 
> Unless I'm missing something, I'd propose this for qemu-stable.

Yes, sounds good.

> 
> Also, mirror with MIRROR_OPEN_BACKING_CHAIN probably has a similar
> problem with opening the images in the backing chain a second time.

Seems so. But if there was such a test case, image locking would have
complained.

Is that a practical use case?  Mirror target image uses the active image as
backign file? (It's a bit late in the evening, please remind me :-)

Fam

Re: [Qemu-devel] [PATCH v15 18/21] block: Reuse bs as backing hd for drive-backup sync=none

[Kevin Wolf]

Am 26.04.2017 um 15:15 hat Fam Zheng geschrieben:
> On Wed, 04/26 14:52, Kevin Wolf wrote:
> > Am 26.04.2017 um 05:34 hat Fam Zheng geschrieben:
> > > Signed-off-by: Fam Zheng <famz@redhat.com>
> > 
> > The commit message is a bit terse. :-)
> > 
> > So I think this means that instead of opening the backing file of the
> > backup (which is probably the active file of the VM) a second time, we
> > instead take a reference on the existing node. Very good idea.
> > 
> > In fact, my question is: How did this ever work? Did we just neglect to
> > test this? The backup backing file will use stale qcow2 metadata when we
> > continue to write to the source.
> 
> Reading from the target BDS would be a problem, but I guess it's relatively
> uncommon:
> 
> - In QEMU code, COW is always done manually in backup_do_cow, so no reading from
>   target->backing in block layer.
> 
> - Writing to target is done in target cluster granularity so no COW too.
> 
> But it's still a fully valid point, for example it can be exported to NBD, etc.

Which I believe is the exact setup for fleecing.

And actually, if we were sure that we never read from the file, we
wouldn't even have to attach the backing file in the first place.

> > Unless I'm missing something, I'd propose this for qemu-stable.
> 
> Yes, sounds good.
> 
> > 
> > Also, mirror with MIRROR_OPEN_BACKING_CHAIN probably has a similar
> > problem with opening the images in the backing chain a second time.
> 
> Seems so. But if there was such a test case, image locking would have
> complained.
> 
> Is that a practical use case?  Mirror target image uses the active image as
> backign file? (It's a bit late in the evening, please remind me :-)

I believe storage migration uses a mode like this, but to be honest I
don't remember the details at the moment either.

But I do see that NEW_IMAGE_MODE_ABSOLUTE_PATHS enters the source as the
backing file of the target and opens it (a second instance of it) when
the mirror completes. Hopefully, by that time the source doesn't have
another user that keeps it alive (and I think op blockers should
actually ensure this), but it looks a bit fishy. Probably worth a closer
look.

Kevin

Re: [Qemu-devel] [PATCH v15 18/21] block: Reuse bs as backing hd for drive-backup sync=none

[Fam Zheng]

On Wed, 04/26 16:34, Kevin Wolf wrote:
> Am 26.04.2017 um 15:15 hat Fam Zheng geschrieben:
> > On Wed, 04/26 14:52, Kevin Wolf wrote:
> > > Am 26.04.2017 um 05:34 hat Fam Zheng geschrieben:
> > > > Signed-off-by: Fam Zheng <famz@redhat.com>
> > > 
> > > The commit message is a bit terse. :-)
> > > 
> > > So I think this means that instead of opening the backing file of the
> > > backup (which is probably the active file of the VM) a second time, we
> > > instead take a reference on the existing node. Very good idea.
> > > 
> > > In fact, my question is: How did this ever work? Did we just neglect to
> > > test this? The backup backing file will use stale qcow2 metadata when we
> > > continue to write to the source.
> > 
> > Reading from the target BDS would be a problem, but I guess it's relatively
> > uncommon:
> > 
> > - In QEMU code, COW is always done manually in backup_do_cow, so no reading from
> >   target->backing in block layer.
> > 
> > - Writing to target is done in target cluster granularity so no COW too.
> > 
> > But it's still a fully valid point, for example it can be exported to NBD, etc.
> 
> Which I believe is the exact setup for fleecing.

Hmm, yes you must be right. Though IMO I've always considered blockdev-add with
backing reference to be the only valid way to do fleecing, this one is IMHO a
hack. And when it was added, there was no way to export it in NBD because it
didn't have a device id.

> 
> And actually, if we were sure that we never read from the file, we
> wouldn't even have to attach the backing file in the first place.

OK, fair enough.

> 
> > > Unless I'm missing something, I'd propose this for qemu-stable.
> > 
> > Yes, sounds good.
> > 
> > > 
> > > Also, mirror with MIRROR_OPEN_BACKING_CHAIN probably has a similar
> > > problem with opening the images in the backing chain a second time.
> > 
> > Seems so. But if there was such a test case, image locking would have
> > complained.
> > 
> > Is that a practical use case?  Mirror target image uses the active image as
> > backign file? (It's a bit late in the evening, please remind me :-)
> 
> I believe storage migration uses a mode like this, but to be honest I
> don't remember the details at the moment either.
> 
> But I do see that NEW_IMAGE_MODE_ABSOLUTE_PATHS enters the source as the
> backing file of the target and opens it (a second instance of it) when
> the mirror completes. Hopefully, by that time the source doesn't have
> another user that keeps it alive (and I think op blockers should
> actually ensure this), but it looks a bit fishy. Probably worth a closer
> look.

Mirror has many graph manipulation magic, which seems even darker than commit.
We should perhaps allocate sometime to see if there are cleaner ways to do them,
with all the new infrastructure we have. Hopefully it's not hard to do, like
this one.

Fam

[Qemu-devel] [PATCH v15 19/21] osdep: Add qemu_lock_fd and qemu_unlock_fd

[Fam Zheng]

They are wrappers of POSIX fcntl "file private locking", with a
convenient "try lock" wrapper implemented with F_OFD_GETLK.

Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 include/qemu/osdep.h |  3 +++
 util/osdep.c         | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 51 insertions(+)

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 122ff06..1c9f5e2 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -341,6 +341,9 @@ int qemu_close(int fd);
 #ifndef _WIN32
 int qemu_dup(int fd);
 #endif
+int qemu_lock_fd(int fd, int64_t start, int64_t len, bool exclusive);
+int qemu_unlock_fd(int fd, int64_t start, int64_t len);
+int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive);
 
 #if defined(__HAIKU__) && defined(__i386__)
 #define FMT_pid "%ld"
diff --git a/util/osdep.c b/util/osdep.c
index 06fb1cf..3de4a18 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -140,6 +140,54 @@ static int qemu_parse_fdset(const char *param)
 {
     return qemu_parse_fd(param);
 }
+
+static int qemu_lock_fcntl(int fd, int64_t start, int64_t len, int fl_type)
+{
+#ifdef F_OFD_SETLK
+    int ret;
+    struct flock fl = {
+        .l_whence = SEEK_SET,
+        .l_start  = start,
+        .l_len    = len,
+        .l_type   = fl_type,
+    };
+    ret = fcntl(fd, F_OFD_SETLK, &fl);
+    return ret == -1 ? -errno : 0;
+#else
+    return -ENOTSUP;
+#endif
+}
+
+int qemu_lock_fd(int fd, int64_t start, int64_t len, bool exclusive)
+{
+    return qemu_lock_fcntl(fd, start, len, exclusive ? F_WRLCK : F_RDLCK);
+}
+
+int qemu_unlock_fd(int fd, int64_t start, int64_t len)
+{
+    return qemu_lock_fcntl(fd, start, len, F_UNLCK);
+}
+
+int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive)
+{
+#ifdef F_OFD_SETLK
+    int ret;
+    struct flock fl = {
+        .l_whence = SEEK_SET,
+        .l_start  = start,
+        .l_len    = len,
+        .l_type   = exclusive ? F_WRLCK : F_RDLCK,
+    };
+    ret = fcntl(fd, F_OFD_GETLK, &fl);
+    if (ret == -1) {
+        return -errno;
+    } else {
+        return fl.l_type == F_UNLCK ? 0 : -EAGAIN;
+    }
+#else
+    return -ENOTSUP;
+#endif
+}
 #endif
 
 /*
-- 
2.9.3

Re: [Qemu-devel] [PATCH v15 19/21] osdep: Add qemu_lock_fd and qemu_unlock_fd

[Kevin Wolf]

Am 26.04.2017 um 05:34 hat Fam Zheng geschrieben:
> They are wrappers of POSIX fcntl "file private locking", with a
> convenient "try lock" wrapper implemented with F_OFD_GETLK.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
>  include/qemu/osdep.h |  3 +++
>  util/osdep.c         | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 51 insertions(+)
> 
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index 122ff06..1c9f5e2 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -341,6 +341,9 @@ int qemu_close(int fd);
>  #ifndef _WIN32
>  int qemu_dup(int fd);
>  #endif
> +int qemu_lock_fd(int fd, int64_t start, int64_t len, bool exclusive);
> +int qemu_unlock_fd(int fd, int64_t start, int64_t len);
> +int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive);

For the record: On IRC, I proposed adding something like the following:

    #ifndef F_OFD_SETLK
    #define F_OFD_SETLK F_SETLK
    #define F_OFD_GETLK F_GETLK
    #endif

F_OFD_* are still relatively new and e.g. RHEL 7 doesn't support it yet.
Using process-based locks is suboptimal because we can easily lose them
earlier than we want, but it's still better than nothing and covers the
common simple cases.

Kevin

Re: [Qemu-devel] [PATCH v15 19/21] osdep: Add qemu_lock_fd and qemu_unlock_fd

[Fam Zheng]

On Wed, 04/26 14:57, Kevin Wolf wrote:
> Am 26.04.2017 um 05:34 hat Fam Zheng geschrieben:
> > They are wrappers of POSIX fcntl "file private locking", with a
> > convenient "try lock" wrapper implemented with F_OFD_GETLK.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > Reviewed-by: Max Reitz <mreitz@redhat.com>
> > ---
> >  include/qemu/osdep.h |  3 +++
> >  util/osdep.c         | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 51 insertions(+)
> > 
> > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > index 122ff06..1c9f5e2 100644
> > --- a/include/qemu/osdep.h
> > +++ b/include/qemu/osdep.h
> > @@ -341,6 +341,9 @@ int qemu_close(int fd);
> >  #ifndef _WIN32
> >  int qemu_dup(int fd);
> >  #endif
> > +int qemu_lock_fd(int fd, int64_t start, int64_t len, bool exclusive);
> > +int qemu_unlock_fd(int fd, int64_t start, int64_t len);
> > +int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive);
> 
> For the record: On IRC, I proposed adding something like the following:
> 
>     #ifndef F_OFD_SETLK
>     #define F_OFD_SETLK F_SETLK
>     #define F_OFD_GETLK F_GETLK
>     #endif
> 
> F_OFD_* are still relatively new and e.g. RHEL 7 doesn't support it yet.
> Using process-based locks is suboptimal because we can easily lose them
> earlier than we want, but it's still better than nothing and covers the
> common simple cases.

Yes, we should add that. But I'd prefer:

    #ifdef F_OFD_SETLK
    #define QEMU_SETLK F_OFD_SETLK
    #define QEMU_GETLK F_OFD_GETLK
    #else
    #define QEMU_SETLK F_SETLK
    #define QEMU_GETLK F_GETLK
    #endif

to avoid "abusing" the macro name.

Another question is whether we should print a warning to make users aware? Even
the test case in patch 21 can see three "lock losses" on RHEL with posix lock,
and there are way more corner cases, I believe.

We can print a warning to stderr in raw_open_common when F_OFD_GETLK is not
available, I think.

Fam

Re: [Qemu-devel] [PATCH v15 19/21] osdep: Add qemu_lock_fd and qemu_unlock_fd

[Kevin Wolf]

Am 26.04.2017 um 15:20 hat Fam Zheng geschrieben:
> On Wed, 04/26 14:57, Kevin Wolf wrote:
> > Am 26.04.2017 um 05:34 hat Fam Zheng geschrieben:
> > > They are wrappers of POSIX fcntl "file private locking", with a
> > > convenient "try lock" wrapper implemented with F_OFD_GETLK.
> > > 
> > > Signed-off-by: Fam Zheng <famz@redhat.com>
> > > Reviewed-by: Max Reitz <mreitz@redhat.com>
> > > ---
> > >  include/qemu/osdep.h |  3 +++
> > >  util/osdep.c         | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 51 insertions(+)
> > > 
> > > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > > index 122ff06..1c9f5e2 100644
> > > --- a/include/qemu/osdep.h
> > > +++ b/include/qemu/osdep.h
> > > @@ -341,6 +341,9 @@ int qemu_close(int fd);
> > >  #ifndef _WIN32
> > >  int qemu_dup(int fd);
> > >  #endif
> > > +int qemu_lock_fd(int fd, int64_t start, int64_t len, bool exclusive);
> > > +int qemu_unlock_fd(int fd, int64_t start, int64_t len);
> > > +int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive);
> > 
> > For the record: On IRC, I proposed adding something like the following:
> > 
> >     #ifndef F_OFD_SETLK
> >     #define F_OFD_SETLK F_SETLK
> >     #define F_OFD_GETLK F_GETLK
> >     #endif
> > 
> > F_OFD_* are still relatively new and e.g. RHEL 7 doesn't support it yet.
> > Using process-based locks is suboptimal because we can easily lose them
> > earlier than we want, but it's still better than nothing and covers the
> > common simple cases.
> 
> Yes, we should add that. But I'd prefer:
> 
>     #ifdef F_OFD_SETLK
>     #define QEMU_SETLK F_OFD_SETLK
>     #define QEMU_GETLK F_OFD_GETLK
>     #else
>     #define QEMU_SETLK F_SETLK
>     #define QEMU_GETLK F_GETLK
>     #endif
> 
> to avoid "abusing" the macro name.

Makes sense.

> Another question is whether we should print a warning to make users
> aware? Even the test case in patch 21 can see three "lock losses" on
> RHEL with posix lock, and there are way more corner cases, I believe.
> 
> We can print a warning to stderr in raw_open_common when F_OFD_GETLK
> is not available, I think.

Yes, this sounds reasonable, too. Dependent on s->use_locks, I guess.

Kevin

Re: [Qemu-devel] [PATCH v15 19/21] osdep: Add qemu_lock_fd and qemu_unlock_fd

[Daniel P. Berrange]

On Wed, Apr 26, 2017 at 09:20:28PM +0800, Fam Zheng wrote:
> On Wed, 04/26 14:57, Kevin Wolf wrote:
> > Am 26.04.2017 um 05:34 hat Fam Zheng geschrieben:
> > > They are wrappers of POSIX fcntl "file private locking", with a
> > > convenient "try lock" wrapper implemented with F_OFD_GETLK.
> > > 
> > > Signed-off-by: Fam Zheng <famz@redhat.com>
> > > Reviewed-by: Max Reitz <mreitz@redhat.com>
> > > ---
> > >  include/qemu/osdep.h |  3 +++
> > >  util/osdep.c         | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 51 insertions(+)
> > > 
> > > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > > index 122ff06..1c9f5e2 100644
> > > --- a/include/qemu/osdep.h
> > > +++ b/include/qemu/osdep.h
> > > @@ -341,6 +341,9 @@ int qemu_close(int fd);
> > >  #ifndef _WIN32
> > >  int qemu_dup(int fd);
> > >  #endif
> > > +int qemu_lock_fd(int fd, int64_t start, int64_t len, bool exclusive);
> > > +int qemu_unlock_fd(int fd, int64_t start, int64_t len);
> > > +int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive);
> > 
> > For the record: On IRC, I proposed adding something like the following:
> > 
> >     #ifndef F_OFD_SETLK
> >     #define F_OFD_SETLK F_SETLK
> >     #define F_OFD_GETLK F_GETLK
> >     #endif
> > 
> > F_OFD_* are still relatively new and e.g. RHEL 7 doesn't support it yet.
> > Using process-based locks is suboptimal because we can easily lose them
> > earlier than we want, but it's still better than nothing and covers the
> > common simple cases.
> 
> Yes, we should add that. But I'd prefer:
> 
>     #ifdef F_OFD_SETLK
>     #define QEMU_SETLK F_OFD_SETLK
>     #define QEMU_GETLK F_OFD_GETLK
>     #else
>     #define QEMU_SETLK F_SETLK
>     #define QEMU_GETLK F_GETLK
>     #endif
> 
> to avoid "abusing" the macro name.
> 
> Another question is whether we should print a warning to make users aware? Even
> the test case in patch 21 can see three "lock losses" on RHEL with posix lock,
> and there are way more corner cases, I believe.

Is it possible to use F_GETLK to detect when we have a missing lock, then
add assertions in various key places that call F_GETLK to validate hat
the lock still exists & print an warning.

I don't like the idea that downstream would be running with locking enabled,
but silently loosing locks with no indication at all that this happened,
especially when upstream development won't be testing this since those devs
will use the F_OFD_SETLK instead.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v15 19/21] osdep: Add qemu_lock_fd and qemu_unlock_fd

[Fam Zheng]

On Wed, 04/26 15:29, Daniel P. Berrange wrote:
> On Wed, Apr 26, 2017 at 09:20:28PM +0800, Fam Zheng wrote:
> > On Wed, 04/26 14:57, Kevin Wolf wrote:
> > > Am 26.04.2017 um 05:34 hat Fam Zheng geschrieben:
> > > > They are wrappers of POSIX fcntl "file private locking", with a
> > > > convenient "try lock" wrapper implemented with F_OFD_GETLK.
> > > > 
> > > > Signed-off-by: Fam Zheng <famz@redhat.com>
> > > > Reviewed-by: Max Reitz <mreitz@redhat.com>
> > > > ---
> > > >  include/qemu/osdep.h |  3 +++
> > > >  util/osdep.c         | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
> > > >  2 files changed, 51 insertions(+)
> > > > 
> > > > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > > > index 122ff06..1c9f5e2 100644
> > > > --- a/include/qemu/osdep.h
> > > > +++ b/include/qemu/osdep.h
> > > > @@ -341,6 +341,9 @@ int qemu_close(int fd);
> > > >  #ifndef _WIN32
> > > >  int qemu_dup(int fd);
> > > >  #endif
> > > > +int qemu_lock_fd(int fd, int64_t start, int64_t len, bool exclusive);
> > > > +int qemu_unlock_fd(int fd, int64_t start, int64_t len);
> > > > +int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive);
> > > 
> > > For the record: On IRC, I proposed adding something like the following:
> > > 
> > >     #ifndef F_OFD_SETLK
> > >     #define F_OFD_SETLK F_SETLK
> > >     #define F_OFD_GETLK F_GETLK
> > >     #endif
> > > 
> > > F_OFD_* are still relatively new and e.g. RHEL 7 doesn't support it yet.
> > > Using process-based locks is suboptimal because we can easily lose them
> > > earlier than we want, but it's still better than nothing and covers the
> > > common simple cases.
> > 
> > Yes, we should add that. But I'd prefer:
> > 
> >     #ifdef F_OFD_SETLK
> >     #define QEMU_SETLK F_OFD_SETLK
> >     #define QEMU_GETLK F_OFD_GETLK
> >     #else
> >     #define QEMU_SETLK F_SETLK
> >     #define QEMU_GETLK F_GETLK
> >     #endif
> > 
> > to avoid "abusing" the macro name.
> > 
> > Another question is whether we should print a warning to make users aware? Even
> > the test case in patch 21 can see three "lock losses" on RHEL with posix lock,
> > and there are way more corner cases, I believe.
> 
> Is it possible to use F_GETLK to detect when we have a missing lock, then
> add assertions in various key places that call F_GETLK to validate hat
> the lock still exists & print an warning.

Possibly, but it's ugly to do, and probably isn't worth the effort in finding
all the places where losses happen and where locks should be checked against
losses. Besides, maybe I'm missing something, it's also hard to use F_GETLK to
find lost locks, because either it's lost or not, it can return F_UNLCK.

> 
> I don't like the idea that downstream would be running with locking enabled,
> but silently loosing locks with no indication at all that this happened,
> especially when upstream development won't be testing this since those devs
> will use the F_OFD_SETLK instead.

Yes, see my reply to Kevin on the last patch: we could default to disabled if
there is no OFD lock (via documented locking=auto, no magic), and avoid
surprising users.

Fam

[Qemu-devel] [PATCH v15 20/21] file-posix: Add image locking to perm operations

[Fam Zheng]

This extends the permission bits of op blocker API to external using
Linux OFD locks.

Each permission in @perm and @shared_perm is represented by a locked
byte in the image file.  Requesting a permission in @perm is translated
to a shared lock of the corresponding byte; rejecting to share the same
permission is translated to a shared lock of a separate byte. With that,
we use 2x number of bytes of distinct permission types.

virtlockd in libvirt locks the first byte, so we do locking from a
higher offset.

Suggested-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/file-posix.c | 267 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 264 insertions(+), 3 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 2114720..b92fdc3 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -129,12 +129,28 @@ do { \
 
 #define MAX_BLOCKSIZE	4096
 
+/* Posix file locking bytes. Libvirt takes byte 0, we start from higher bytes,
+ * leaving a few more bytes for its future use. */
+#define RAW_LOCK_PERM_BASE             100
+#define RAW_LOCK_SHARED_BASE           200
+#ifdef F_OFD_SETLK
+#define RAW_LOCK_SUPPORTED 1
+#else
+#define RAW_LOCK_SUPPORTED 0
+#endif
+
 typedef struct BDRVRawState {
     int fd;
+    int lock_fd;
+    bool use_lock;
     int type;
     int open_flags;
     size_t buf_align;
 
+    /* The current permissions. */
+    uint64_t perm;
+    uint64_t shared_perm;
+
 #ifdef CONFIG_XFS
     bool is_xfs:1;
 #endif
@@ -440,6 +456,8 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
     }
     s->use_linux_aio = (aio == BLOCKDEV_AIO_OPTIONS_NATIVE);
 
+    s->use_lock = qemu_opt_get_bool(opts, "locking", true);
+
     s->open_flags = open_flags;
     raw_parse_flags(bdrv_flags, &s->open_flags);
 
@@ -455,6 +473,21 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
     }
     s->fd = fd;
 
+    s->lock_fd = -1;
+    fd = qemu_open(filename, O_RDONLY);
+    if (fd < 0) {
+        if (RAW_LOCK_SUPPORTED) {
+            ret = -errno;
+            error_setg_errno(errp, errno, "Could not open '%s' for locking",
+                             filename);
+            qemu_close(s->fd);
+            goto fail;
+        }
+    }
+    s->lock_fd = fd;
+    s->perm = 0;
+    s->shared_perm = BLK_PERM_ALL;
+
 #ifdef CONFIG_LINUX_AIO
      /* Currently Linux does AIO only for files opened with O_DIRECT */
     if (s->use_linux_aio && !(s->open_flags & O_DIRECT)) {
@@ -542,6 +575,156 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags,
     return raw_open_common(bs, options, flags, 0, errp);
 }
 
+typedef enum {
+    RAW_PL_PREPARE,
+    RAW_PL_COMMIT,
+    RAW_PL_ABORT,
+} RawPermLockOp;
+
+/* Lock wanted bytes by @perm and ~@shared_perm in the file; if @unlock ==
+ * true, also unlock the unneeded bytes. */
+static int raw_apply_lock_bytes(BDRVRawState *s,
+                                uint64_t perm_lock_bits,
+                                uint64_t shared_perm_lock_bits,
+                                bool unlock, Error **errp)
+{
+    int ret;
+    int i;
+
+    for (i = 0; i < BLK_PERM_MAX; ++i) {
+        int off = RAW_LOCK_PERM_BASE + i;
+        if (perm_lock_bits & (1ULL << i)) {
+            ret = qemu_lock_fd(s->lock_fd, off, 1, false);
+            if (ret) {
+                error_setg(errp, "Failed to lock byte %d", off);
+                return ret;
+            }
+        } else if (unlock) {
+            ret = qemu_unlock_fd(s->lock_fd, off, 1);
+            if (ret) {
+                error_setg(errp, "Failed to unlock byte %d", off);
+                return ret;
+            }
+        }
+    }
+    for (i = 0; i < BLK_PERM_MAX; ++i) {
+        int off = RAW_LOCK_SHARED_BASE + i;
+        if (shared_perm_lock_bits & (1ULL << i)) {
+            ret = qemu_lock_fd(s->lock_fd, off, 1, false);
+            if (ret) {
+                error_setg(errp, "Failed to lock byte %d", off);
+                return ret;
+            }
+        } else if (unlock) {
+            ret = qemu_unlock_fd(s->lock_fd, off, 1);
+            if (ret) {
+                error_setg(errp, "Failed to unlock byte %d", off);
+                return ret;
+            }
+        }
+    }
+    return 0;
+}
+
+/* Check "unshared" bytes implied by @perm and ~@shared_perm in the file. */
+static int raw_check_lock_bytes(BDRVRawState *s,
+                                uint64_t perm, uint64_t shared_perm,
+                                Error **errp)
+{
+    int ret;
+    int i;
+
+    for (i = 0; i < BLK_PERM_MAX; ++i) {
+        int off = RAW_LOCK_SHARED_BASE + i;
+        uint64_t p = 1ULL << i;
+        if (perm & p) {
+            ret = qemu_lock_fd_test(s->lock_fd, off, 1, true);
+            if (ret) {
+                error_setg(errp,
+                           "Failed to check byte %d for \"%s\" permission",
+                           off, bdrv_perm_names(p));
+                error_append_hint(errp,
+                                  "Is another process using the image?\n");
+                return ret;
+            }
+        }
+    }
+    for (i = 0; i < BLK_PERM_MAX; ++i) {
+        int off = RAW_LOCK_PERM_BASE + i;
+        uint64_t p = 1ULL << i;
+        if (!(shared_perm & p)) {
+            ret = qemu_lock_fd_test(s->lock_fd, off, 1, true);
+            if (ret) {
+                error_setg(errp,
+                           "Failed to check byte %d for shared \"%s\" permission",
+                           off, bdrv_perm_names(p));
+                error_append_hint(errp,
+                                  "Is another process using the image?\n");
+                return ret;
+            }
+        }
+    }
+    return 0;
+}
+
+static int raw_handle_perm_lock(BlockDriverState *bs,
+                                RawPermLockOp op,
+                                uint64_t new_perm, uint64_t new_shared,
+                                Error **errp)
+{
+    BDRVRawState *s = bs->opaque;
+    int ret = 0;
+    Error *local_err = NULL;
+
+    if (!RAW_LOCK_SUPPORTED) {
+        return 0;
+    }
+
+    if (!s->use_lock) {
+        return 0;
+    }
+
+    if (bdrv_get_flags(bs) & BDRV_O_INACTIVE) {
+        return 0;
+    }
+
+    assert(s->lock_fd > 0);
+
+    switch (op) {
+    case RAW_PL_PREPARE:
+        ret = raw_apply_lock_bytes(s, s->perm | new_perm,
+                                   ~s->shared_perm | ~new_shared,
+                                   false, errp);
+        if (!ret) {
+            ret = raw_check_lock_bytes(s, new_perm, new_shared, errp);
+            if (!ret) {
+                return 0;
+            }
+        }
+        op = RAW_PL_ABORT;
+        /* fall through to unlock bytes. */
+    case RAW_PL_ABORT:
+        raw_apply_lock_bytes(s, s->perm, ~s->shared_perm, true, &local_err);
+        if (local_err) {
+            /* Theoretically the above call only unlocks bytes and it cannot
+             * fail. Something weird happened, report it.
+             */
+            error_report_err(local_err);
+        }
+        break;
+    case RAW_PL_COMMIT:
+        raw_apply_lock_bytes(s, new_perm, ~new_shared, true, &local_err);
+        if (local_err) {
+            /* Theoretically the above call only unlocks bytes and it cannot
+             * fail. Something weird happened, report it.
+             */
+            error_report_err(local_err);
+        }
+        break;
+    }
+    return ret;
+}
+
 static int raw_reopen_prepare(BDRVReopenState *state,
                               BlockReopenQueue *queue, Error **errp)
 {
@@ -549,6 +732,8 @@ static int raw_reopen_prepare(BDRVReopenState *state,
     BDRVRawReopenState *rs;
     int ret = 0;
     Error *local_err = NULL;
+    uint64_t clear_perms = state->flags & BDRV_O_RDWR ? 0 :
+        BLK_PERM_WRITE | BLK_PERM_RESIZE | BLK_PERM_WRITE_UNCHANGED;
 
     assert(state != NULL);
     assert(state->bs != NULL);
@@ -613,13 +798,22 @@ static int raw_reopen_prepare(BDRVReopenState *state,
     if (rs->fd != -1) {
         raw_probe_alignment(state->bs, rs->fd, &local_err);
         if (local_err) {
-            qemu_close(rs->fd);
-            rs->fd = -1;
             error_propagate(errp, local_err);
             ret = -EINVAL;
+            goto fail;
         }
     }
 
+    ret = raw_handle_perm_lock(state->bs, RAW_PL_PREPARE,
+                               s->perm & ~clear_perms,
+                               s->shared_perm, errp);
+    if (ret) {
+        goto fail;
+    }
+    return 0;
+fail:
+    qemu_close(rs->fd);
+    rs->fd = -1;
     return ret;
 }
 
@@ -627,6 +821,8 @@ static void raw_reopen_commit(BDRVReopenState *state)
 {
     BDRVRawReopenState *rs = state->opaque;
     BDRVRawState *s = state->bs->opaque;
+    uint64_t clear_perms = state->flags & BDRV_O_RDWR ? 0 :
+        BLK_PERM_WRITE | BLK_PERM_RESIZE | BLK_PERM_WRITE_UNCHANGED;
 
     s->open_flags = rs->open_flags;
 
@@ -635,12 +831,17 @@ static void raw_reopen_commit(BDRVReopenState *state)
 
     g_free(state->opaque);
     state->opaque = NULL;
+    raw_handle_perm_lock(state->bs, RAW_PL_COMMIT, s->perm & ~clear_perms,
+                         s->shared_perm, NULL);
 }
 
 
 static void raw_reopen_abort(BDRVReopenState *state)
 {
+    BDRVRawState *s = state->bs->opaque;
     BDRVRawReopenState *rs = state->opaque;
+    uint64_t clear_perms = state->flags & BDRV_O_RDWR ? 0 :
+        BLK_PERM_WRITE | BLK_PERM_RESIZE | BLK_PERM_WRITE_UNCHANGED;
 
      /* nothing to do if NULL, we didn't get far enough */
     if (rs == NULL) {
@@ -653,6 +854,8 @@ static void raw_reopen_abort(BDRVReopenState *state)
     }
     g_free(state->opaque);
     state->opaque = NULL;
+    raw_handle_perm_lock(state->bs, RAW_PL_ABORT, s->perm & ~clear_perms,
+                         s->shared_perm, NULL);
 }
 
 static int hdev_get_max_transfer_length(BlockDriverState *bs, int fd)
@@ -1410,6 +1613,10 @@ static void raw_close(BlockDriverState *bs)
         qemu_close(s->fd);
         s->fd = -1;
     }
+    if (s->lock_fd >= 0) {
+        qemu_close(s->lock_fd);
+        s->lock_fd = -1;
+    }
 }
 
 static int raw_truncate(BlockDriverState *bs, int64_t offset)
@@ -1947,6 +2154,56 @@ static QemuOptsList raw_create_opts = {
     }
 };
 
+static int raw_check_perm(BlockDriverState *bs, uint64_t perm, uint64_t shared,
+                          Error **errp)
+{
+    return raw_handle_perm_lock(bs, RAW_PL_PREPARE, perm, shared, errp);
+}
+
+static void raw_set_perm(BlockDriverState *bs, uint64_t perm, uint64_t shared)
+{
+    BDRVRawState *s = bs->opaque;
+    raw_handle_perm_lock(bs, RAW_PL_COMMIT, perm, shared, NULL);
+    s->perm = perm;
+    s->shared_perm = shared;
+}
+
+static void raw_abort_perm_update(BlockDriverState *bs)
+{
+    BDRVRawState *s = bs->opaque;
+
+    raw_handle_perm_lock(bs, RAW_PL_ABORT, s->perm, s->shared_perm, NULL);
+}
+
+static int raw_inactivate(BlockDriverState *bs)
+{
+    int ret;
+    uint64_t perm = 0;
+    uint64_t shared = BLK_PERM_ALL;
+
+    ret = raw_handle_perm_lock(bs, RAW_PL_PREPARE, perm, shared, NULL);
+    if (ret) {
+        return ret;
+    }
+    raw_handle_perm_lock(bs, RAW_PL_COMMIT, perm, shared, NULL);
+    return 0;
+}
+
+
+static void raw_invalidate_cache(BlockDriverState *bs, Error **errp)
+{
+    BDRVRawState *s = bs->opaque;
+    int ret;
+
+    assert(!(bdrv_get_flags(bs) & BDRV_O_INACTIVE));
+    ret = raw_handle_perm_lock(bs, RAW_PL_PREPARE, s->perm, s->shared_perm,
+                               errp);
+    if (ret) {
+        return;
+    }
+    raw_handle_perm_lock(bs, RAW_PL_COMMIT, s->perm, s->shared_perm, NULL);
+}
+
 BlockDriver bdrv_file = {
     .format_name = "file",
     .protocol_name = "file",
@@ -1977,7 +2234,11 @@ BlockDriver bdrv_file = {
     .bdrv_get_info = raw_get_info,
     .bdrv_get_allocated_file_size
                         = raw_get_allocated_file_size,
-
+    .bdrv_inactivate = raw_inactivate,
+    .bdrv_invalidate_cache = raw_invalidate_cache,
+    .bdrv_check_perm = raw_check_perm,
+    .bdrv_set_perm   = raw_set_perm,
+    .bdrv_abort_perm_update = raw_abort_perm_update,
     .create_opts = &raw_create_opts,
 };
 
-- 
2.9.3

Re: [Qemu-devel] [PATCH v15 20/21] file-posix: Add image locking to perm operations

[Kevin Wolf]

Am 26.04.2017 um 05:34 hat Fam Zheng geschrieben:
> This extends the permission bits of op blocker API to external using
> Linux OFD locks.
> 
> Each permission in @perm and @shared_perm is represented by a locked
> byte in the image file.  Requesting a permission in @perm is translated
> to a shared lock of the corresponding byte; rejecting to share the same
> permission is translated to a shared lock of a separate byte. With that,
> we use 2x number of bytes of distinct permission types.
> 
> virtlockd in libvirt locks the first byte, so we do locking from a
> higher offset.
> 
> Suggested-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/file-posix.c | 267 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 264 insertions(+), 3 deletions(-)
> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 2114720..b92fdc3 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -129,12 +129,28 @@ do { \
>  
>  #define MAX_BLOCKSIZE	4096
>  
> +/* Posix file locking bytes. Libvirt takes byte 0, we start from higher bytes,
> + * leaving a few more bytes for its future use. */
> +#define RAW_LOCK_PERM_BASE             100
> +#define RAW_LOCK_SHARED_BASE           200
> +#ifdef F_OFD_SETLK
> +#define RAW_LOCK_SUPPORTED 1
> +#else
> +#define RAW_LOCK_SUPPORTED 0
> +#endif
> +
>  typedef struct BDRVRawState {
>      int fd;
> +    int lock_fd;
> +    bool use_lock;
>      int type;
>      int open_flags;
>      size_t buf_align;
>  
> +    /* The current permissions. */
> +    uint64_t perm;
> +    uint64_t shared_perm;
> +
>  #ifdef CONFIG_XFS
>      bool is_xfs:1;
>  #endif
> @@ -440,6 +456,8 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
>      }
>      s->use_linux_aio = (aio == BLOCKDEV_AIO_OPTIONS_NATIVE);
>  
> +    s->use_lock = qemu_opt_get_bool(opts, "locking", true);
> +
>      s->open_flags = open_flags;
>      raw_parse_flags(bdrv_flags, &s->open_flags);
>  
> @@ -455,6 +473,21 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
>      }
>      s->fd = fd;
>  
> +    s->lock_fd = -1;
> +    fd = qemu_open(filename, O_RDONLY);

Note that with /dev/fdset there can be cases where we can open a file
O_RDWR, but not O_RDONLY. Should we better just use the same flags as
for the s->fd?

> +    if (fd < 0) {
> +        if (RAW_LOCK_SUPPORTED) {
> +            ret = -errno;
> +            error_setg_errno(errp, errno, "Could not open '%s' for locking",
> +                             filename);
> +            qemu_close(s->fd);
> +            goto fail;
> +        }
> +    }

You still open the fd and possibly error out even with s->use_lock ==
false. Should the code starting from qemu_open() to here be conditional
on s->use_lock?

> +    s->lock_fd = fd;
> +    s->perm = 0;
> +    s->shared_perm = BLK_PERM_ALL;
> +
>  #ifdef CONFIG_LINUX_AIO
>       /* Currently Linux does AIO only for files opened with O_DIRECT */
>      if (s->use_linux_aio && !(s->open_flags & O_DIRECT)) {
> @@ -542,6 +575,156 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags,
>      return raw_open_common(bs, options, flags, 0, errp);
>  }
>  
> +typedef enum {
> +    RAW_PL_PREPARE,
> +    RAW_PL_COMMIT,
> +    RAW_PL_ABORT,
> +} RawPermLockOp;
> +
> +/* Lock wanted bytes by @perm and ~@shared_perm in the file; if @unlock ==

This function doesn't take @perm or @shared_perm. This comment is
especially confusing because shared_perm_lock_bits == ~shared_perm. We
should be explicit here that shared_perm_lock_bits is the mask of all
permissions that _cannot_ be shared.

> + * true, also unlock the unneeded bytes. */
> +static int raw_apply_lock_bytes(BDRVRawState *s,
> +                                uint64_t perm_lock_bits,
> +                                uint64_t shared_perm_lock_bits,
> +                                bool unlock, Error **errp)
> +{
> +    int ret;
> +    int i;
> +
> +    for (i = 0; i < BLK_PERM_MAX; ++i) {
> +        int off = RAW_LOCK_PERM_BASE + i;
> +        if (perm_lock_bits & (1ULL << i)) {
> +            ret = qemu_lock_fd(s->lock_fd, off, 1, false);
> +            if (ret) {
> +                error_setg(errp, "Failed to lock byte %d", off);

Should bdrv_perm_names() be used in this function, too? Maybe not that
important because we never expect this to fail (we don't do any
exclusive locks).

> +                return ret;
> +            }
> +        } else if (unlock) {
> +            ret = qemu_unlock_fd(s->lock_fd, off, 1);
> +            if (ret) {
> +                error_setg(errp, "Failed to unlock byte %d", off);
> +                return ret;
> +            }
> +        }
> +    }
> +    for (i = 0; i < BLK_PERM_MAX; ++i) {
> +        int off = RAW_LOCK_SHARED_BASE + i;
> +        if (shared_perm_lock_bits & (1ULL << i)) {
> +            ret = qemu_lock_fd(s->lock_fd, off, 1, false);
> +            if (ret) {
> +                error_setg(errp, "Failed to lock byte %d", off);
> +                return ret;
> +            }
> +        } else if (unlock) {
> +            ret = qemu_unlock_fd(s->lock_fd, off, 1);
> +            if (ret) {
> +                error_setg(errp, "Failed to unlock byte %d", off);
> +                return ret;
> +            }
> +        }
> +    }
> +    return 0;
> +}

Note: If this function returns an error, we may have acquired some of
the locks. The caller probably wants to call it again to reset the
permissions to the old mask.

> +/* Check "unshared" bytes implied by @perm and ~@shared_perm in the file. */
> +static int raw_check_lock_bytes(BDRVRawState *s,
> +                                uint64_t perm, uint64_t shared_perm,
> +                                Error **errp)
> +{
> +    int ret;
> +    int i;
> +
> +    for (i = 0; i < BLK_PERM_MAX; ++i) {
> +        int off = RAW_LOCK_SHARED_BASE + i;
> +        uint64_t p = 1ULL << i;
> +        if (perm & p) {
> +            ret = qemu_lock_fd_test(s->lock_fd, off, 1, true);
> +            if (ret) {
> +                error_setg(errp,
> +                           "Failed to check byte %d for \"%s\" permission",
> +                           off, bdrv_perm_names(p));

bdrv_perm_names() returns a malloced string, which is leaked here.

> +                error_append_hint(errp,
> +                                  "Is another process using the image?\n");

We probably need to tweak the error messages a bit. When I saw this one
this morning, I felt that if I didn't know how you were going to
implement the locking, it would make zero sense to me:

$ ./qemu-img snapshot -l /tmp/test.qcow2
qemu-img: Could not open '/tmp/test.qcow2': Failed to check byte 101 for shared "write" permission
Is another process using the image?

Something shorter and less technical like 'Failed to get "write" lock'
is probably the friendlier message.

> +                return ret;
> +            }
> +        }
> +    }
> +    for (i = 0; i < BLK_PERM_MAX; ++i) {
> +        int off = RAW_LOCK_PERM_BASE + i;
> +        uint64_t p = 1ULL << i;
> +        if (!(shared_perm & p)) {
> +            ret = qemu_lock_fd_test(s->lock_fd, off, 1, true);
> +            if (ret) {
> +                error_setg(errp,
> +                           "Failed to check byte %d for shared \"%s\" permission",
> +                           off, bdrv_perm_names(p));
> +                error_append_hint(errp,
> +                                  "Is another process using the image?\n");
> +                return ret;
> +            }
> +        }
> +    }
> +    return 0;
> +}
> +
> +static int raw_handle_perm_lock(BlockDriverState *bs,
> +                                RawPermLockOp op,
> +                                uint64_t new_perm, uint64_t new_shared,
> +                                Error **errp)
> +{
> +    BDRVRawState *s = bs->opaque;
> +    int ret = 0;
> +    Error *local_err = NULL;
> +
> +    if (!RAW_LOCK_SUPPORTED) {
> +        return 0;
> +    }
> +
> +    if (!s->use_lock) {
> +        return 0;
> +    }
> +
> +    if (bdrv_get_flags(bs) & BDRV_O_INACTIVE) {
> +        return 0;
> +    }

I don't like the handling of BDRV_O_INACTIVE here in the file-posix
driver. In theory, the users of the node already shouldn't be requesting
any problematic permissions while the image is inactive.

What are the actual problems that we're solving with this and the
.bdrv_invalidate_cache/.bdrv_inactivate callbacks?

> +    assert(s->lock_fd > 0);
> +
> +    switch (op) {
> +    case RAW_PL_PREPARE:
> +        ret = raw_apply_lock_bytes(s, s->perm | new_perm,
> +                                   ~s->shared_perm | ~new_shared,
> +                                   false, errp);
> +        if (!ret) {
> +            ret = raw_check_lock_bytes(s, new_perm, new_shared, errp);
> +            if (!ret) {
> +                return 0;
> +            }
> +        }
> +        op = RAW_PL_ABORT;

op isn't used after this place, I wouldn't be surprised if some compiler
or static analyser complained about it.

> +        /* fall through to unlock bytes. */

Good, this undoes whatever raw_apply_lock_bytes() already has done.

> +    case RAW_PL_ABORT:
> +        raw_apply_lock_bytes(s, s->perm, ~s->shared_perm, true, &local_err);
> +        if (local_err) {
> +            /* Theoretically the above call only unlocks bytes and it cannot
> +             * fail. Something weird happened, report it.
> +             */
> +            error_report_err(local_err);
> +        }
> +        break;
> +    case RAW_PL_COMMIT:
> +        raw_apply_lock_bytes(s, new_perm, ~new_shared, true, &local_err);
> +        if (local_err) {
> +            /* Theoretically the above call only unlocks bytes and it cannot
> +             * fail. Something weird happened, report it.
> +             */
> +            error_report_err(local_err);
> +        }
> +        break;
> +    }
> +    return ret;
> +}
> +
>  static int raw_reopen_prepare(BDRVReopenState *state,
>                                BlockReopenQueue *queue, Error **errp)
>  {
> @@ -549,6 +732,8 @@ static int raw_reopen_prepare(BDRVReopenState *state,
>      BDRVRawReopenState *rs;
>      int ret = 0;
>      Error *local_err = NULL;
> +    uint64_t clear_perms = state->flags & BDRV_O_RDWR ? 0 :
> +        BLK_PERM_WRITE | BLK_PERM_RESIZE | BLK_PERM_WRITE_UNCHANGED;
>  
>      assert(state != NULL);
>      assert(state->bs != NULL);
> @@ -613,13 +798,22 @@ static int raw_reopen_prepare(BDRVReopenState *state,
>      if (rs->fd != -1) {
>          raw_probe_alignment(state->bs, rs->fd, &local_err);
>          if (local_err) {
> -            qemu_close(rs->fd);
> -            rs->fd = -1;
>              error_propagate(errp, local_err);
>              ret = -EINVAL;
> +            goto fail;
>          }
>      }
>  
> +    ret = raw_handle_perm_lock(state->bs, RAW_PL_PREPARE,
> +                               s->perm & ~clear_perms,
> +                               s->shared_perm, errp);

Is this a workaround for bdrv_can_set_read_only() not checking whether
there are still writers attached? Because I think in theory, we should
be able to assert() that clear_perms are already clear.

> +    if (ret) {
> +        goto fail;
> +    }
> +    return 0;
> +fail:
> +    qemu_close(rs->fd);
> +    rs->fd = -1;
>      return ret;
>  }
>  
> @@ -627,6 +821,8 @@ static void raw_reopen_commit(BDRVReopenState *state)
>  {
>      BDRVRawReopenState *rs = state->opaque;
>      BDRVRawState *s = state->bs->opaque;
> +    uint64_t clear_perms = state->flags & BDRV_O_RDWR ? 0 :
> +        BLK_PERM_WRITE | BLK_PERM_RESIZE | BLK_PERM_WRITE_UNCHANGED;
>  
>      s->open_flags = rs->open_flags;
>  
> @@ -635,12 +831,17 @@ static void raw_reopen_commit(BDRVReopenState *state)
>  
>      g_free(state->opaque);
>      state->opaque = NULL;
> +    raw_handle_perm_lock(state->bs, RAW_PL_COMMIT, s->perm & ~clear_perms,
> +                         s->shared_perm, NULL);

Shouldn't we update s->perm here? Or probably move the update into
raw_handle_perm_lock(). Or even more probably, as said above, replace
the permission handling in raw_reopen_* by checking permissions in the
generic block layer beforehand.

>  }
>  
>  
>  static void raw_reopen_abort(BDRVReopenState *state)
>  {
> +    BDRVRawState *s = state->bs->opaque;
>      BDRVRawReopenState *rs = state->opaque;
> +    uint64_t clear_perms = state->flags & BDRV_O_RDWR ? 0 :
> +        BLK_PERM_WRITE | BLK_PERM_RESIZE | BLK_PERM_WRITE_UNCHANGED;
>  
>       /* nothing to do if NULL, we didn't get far enough */
>      if (rs == NULL) {
> @@ -653,6 +854,8 @@ static void raw_reopen_abort(BDRVReopenState *state)
>      }
>      g_free(state->opaque);
>      state->opaque = NULL;
> +    raw_handle_perm_lock(state->bs, RAW_PL_ABORT, s->perm & ~clear_perms,
> +                         s->shared_perm, NULL);
>  }
>  
>  static int hdev_get_max_transfer_length(BlockDriverState *bs, int fd)
> @@ -1410,6 +1613,10 @@ static void raw_close(BlockDriverState *bs)
>          qemu_close(s->fd);
>          s->fd = -1;
>      }
> +    if (s->lock_fd >= 0) {
> +        qemu_close(s->lock_fd);
> +        s->lock_fd = -1;
> +    }
>  }
>  
>  static int raw_truncate(BlockDriverState *bs, int64_t offset)
> @@ -1947,6 +2154,56 @@ static QemuOptsList raw_create_opts = {
>      }
>  };
>  
> +static int raw_check_perm(BlockDriverState *bs, uint64_t perm, uint64_t shared,
> +                          Error **errp)
> +{
> +    return raw_handle_perm_lock(bs, RAW_PL_PREPARE, perm, shared, errp);
> +}
> +
> +static void raw_set_perm(BlockDriverState *bs, uint64_t perm, uint64_t shared)
> +{
> +    BDRVRawState *s = bs->opaque;
> +    raw_handle_perm_lock(bs, RAW_PL_COMMIT, perm, shared, NULL);
> +    s->perm = perm;
> +    s->shared_perm = shared;
> +}
> +
> +static void raw_abort_perm_update(BlockDriverState *bs)
> +{
> +    BDRVRawState *s = bs->opaque;
> +
> +    raw_handle_perm_lock(bs, RAW_PL_ABORT, s->perm, s->shared_perm, NULL);

The parameters are supposed to be the new permissions, which are
obviously ignored in the case of RAW_PL_ABORT. Would passing 0 for
both be more obvious?

> +}
> +
> +static int raw_inactivate(BlockDriverState *bs)
> +{
> +    int ret;
> +    uint64_t perm = 0;
> +    uint64_t shared = BLK_PERM_ALL;
> +
> +    ret = raw_handle_perm_lock(bs, RAW_PL_PREPARE, perm, shared, NULL);
> +    if (ret) {
> +        return ret;
> +    }
> +    raw_handle_perm_lock(bs, RAW_PL_COMMIT, perm, shared, NULL);
> +    return 0;
> +}
> +
> +
> +static void raw_invalidate_cache(BlockDriverState *bs, Error **errp)
> +{
> +    BDRVRawState *s = bs->opaque;
> +    int ret;
> +
> +    assert(!(bdrv_get_flags(bs) & BDRV_O_INACTIVE));
> +    ret = raw_handle_perm_lock(bs, RAW_PL_PREPARE, s->perm, s->shared_perm,
> +                               errp);
> +    if (ret) {
> +        return;
> +    }
> +    raw_handle_perm_lock(bs, RAW_PL_COMMIT, s->perm, s->shared_perm, NULL);
> +}

Looks okay if we really need BDRV_O_INACTIVE handling in file-posix.

Kevin

Re: [Qemu-devel] [PATCH v15 20/21] file-posix: Add image locking to perm operations

[Fam Zheng]

On Wed, 04/26 16:22, Kevin Wolf wrote:
> > @@ -455,6 +473,21 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
> >      }
> >      s->fd = fd;
> >  
> > +    s->lock_fd = -1;
> > +    fd = qemu_open(filename, O_RDONLY);
> 
> Note that with /dev/fdset there can be cases where we can open a file
> O_RDWR, but not O_RDONLY. Should we better just use the same flags as
> for the s->fd?

Makes sense.

> 
> > +    if (fd < 0) {
> > +        if (RAW_LOCK_SUPPORTED) {
> > +            ret = -errno;
> > +            error_setg_errno(errp, errno, "Could not open '%s' for locking",
> > +                             filename);
> > +            qemu_close(s->fd);
> > +            goto fail;
> > +        }
> > +    }
> 
> You still open the fd and possibly error out even with s->use_lock ==
> false. Should the code starting from qemu_open() to here be conditional
> on s->use_lock?

Yes.

> 
> > +    s->lock_fd = fd;
> > +    s->perm = 0;
> > +    s->shared_perm = BLK_PERM_ALL;
> > +
> >  #ifdef CONFIG_LINUX_AIO
> >       /* Currently Linux does AIO only for files opened with O_DIRECT */
> >      if (s->use_linux_aio && !(s->open_flags & O_DIRECT)) {
> > @@ -542,6 +575,156 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags,
> >      return raw_open_common(bs, options, flags, 0, errp);
> >  }
> >  
> > +typedef enum {
> > +    RAW_PL_PREPARE,
> > +    RAW_PL_COMMIT,
> > +    RAW_PL_ABORT,
> > +} RawPermLockOp;
> > +
> > +/* Lock wanted bytes by @perm and ~@shared_perm in the file; if @unlock ==
> 
> This function doesn't take @perm or @shared_perm. This comment is
> especially confusing because shared_perm_lock_bits == ~shared_perm. We
> should be explicit here that shared_perm_lock_bits is the mask of all
> permissions that _cannot_ be shared.

Will update the comment.

> 
> > + * true, also unlock the unneeded bytes. */
> > +static int raw_apply_lock_bytes(BDRVRawState *s,
> > +                                uint64_t perm_lock_bits,
> > +                                uint64_t shared_perm_lock_bits,
> > +                                bool unlock, Error **errp)
> > +{
> > +    int ret;
> > +    int i;
> > +
> > +    for (i = 0; i < BLK_PERM_MAX; ++i) {
> > +        int off = RAW_LOCK_PERM_BASE + i;
> > +        if (perm_lock_bits & (1ULL << i)) {
> > +            ret = qemu_lock_fd(s->lock_fd, off, 1, false);
> > +            if (ret) {
> > +                error_setg(errp, "Failed to lock byte %d", off);
> 
> Should bdrv_perm_names() be used in this function, too? Maybe not that
> important because we never expect this to fail (we don't do any
> exclusive locks).

Ineed, so going a bit of low level is probably better when it does fail. :)

> 
> > +                return ret;
> > +            }
> > +        } else if (unlock) {
> > +            ret = qemu_unlock_fd(s->lock_fd, off, 1);
> > +            if (ret) {
> > +                error_setg(errp, "Failed to unlock byte %d", off);
> > +                return ret;
> > +            }
> > +        }
> > +    }
> > +    for (i = 0; i < BLK_PERM_MAX; ++i) {
> > +        int off = RAW_LOCK_SHARED_BASE + i;
> > +        if (shared_perm_lock_bits & (1ULL << i)) {
> > +            ret = qemu_lock_fd(s->lock_fd, off, 1, false);
> > +            if (ret) {
> > +                error_setg(errp, "Failed to lock byte %d", off);
> > +                return ret;
> > +            }
> > +        } else if (unlock) {
> > +            ret = qemu_unlock_fd(s->lock_fd, off, 1);
> > +            if (ret) {
> > +                error_setg(errp, "Failed to unlock byte %d", off);
> > +                return ret;
> > +            }
> > +        }
> > +    }
> > +    return 0;
> > +}
> 
> Note: If this function returns an error, we may have acquired some of
> the locks. The caller probably wants to call it again to reset the
> permissions to the old mask.

I think callers do.

> 
> > +/* Check "unshared" bytes implied by @perm and ~@shared_perm in the file. */
> > +static int raw_check_lock_bytes(BDRVRawState *s,
> > +                                uint64_t perm, uint64_t shared_perm,
> > +                                Error **errp)
> > +{
> > +    int ret;
> > +    int i;
> > +
> > +    for (i = 0; i < BLK_PERM_MAX; ++i) {
> > +        int off = RAW_LOCK_SHARED_BASE + i;
> > +        uint64_t p = 1ULL << i;
> > +        if (perm & p) {
> > +            ret = qemu_lock_fd_test(s->lock_fd, off, 1, true);
> > +            if (ret) {
> > +                error_setg(errp,
> > +                           "Failed to check byte %d for \"%s\" permission",
> > +                           off, bdrv_perm_names(p));
> 
> bdrv_perm_names() returns a malloced string, which is leaked here.

Neglected that, will fix.

> 
> > +                error_append_hint(errp,
> > +                                  "Is another process using the image?\n");
> 
> We probably need to tweak the error messages a bit. When I saw this one
> this morning, I felt that if I didn't know how you were going to
> implement the locking, it would make zero sense to me:
> 
> $ ./qemu-img snapshot -l /tmp/test.qcow2
> qemu-img: Could not open '/tmp/test.qcow2': Failed to check byte 101 for shared "write" permission
> Is another process using the image?
> 
> Something shorter and less technical like 'Failed to get "write" lock'
> is probably the friendlier message.

OK, it's shorter and friendlier.

> 
> > +                return ret;
> > +            }
> > +        }
> > +    }
> > +    for (i = 0; i < BLK_PERM_MAX; ++i) {
> > +        int off = RAW_LOCK_PERM_BASE + i;
> > +        uint64_t p = 1ULL << i;
> > +        if (!(shared_perm & p)) {
> > +            ret = qemu_lock_fd_test(s->lock_fd, off, 1, true);
> > +            if (ret) {
> > +                error_setg(errp,
> > +                           "Failed to check byte %d for shared \"%s\" permission",
> > +                           off, bdrv_perm_names(p));
> > +                error_append_hint(errp,
> > +                                  "Is another process using the image?\n");
> > +                return ret;
> > +            }
> > +        }
> > +    }
> > +    return 0;
> > +}
> > +
> > +static int raw_handle_perm_lock(BlockDriverState *bs,
> > +                                RawPermLockOp op,
> > +                                uint64_t new_perm, uint64_t new_shared,
> > +                                Error **errp)
> > +{
> > +    BDRVRawState *s = bs->opaque;
> > +    int ret = 0;
> > +    Error *local_err = NULL;
> > +
> > +    if (!RAW_LOCK_SUPPORTED) {
> > +        return 0;
> > +    }
> > +
> > +    if (!s->use_lock) {
> > +        return 0;
> > +    }
> > +
> > +    if (bdrv_get_flags(bs) & BDRV_O_INACTIVE) {
> > +        return 0;
> > +    }
> 
> I don't like the handling of BDRV_O_INACTIVE here in the file-posix
> driver. In theory, the users of the node already shouldn't be requesting
> any problematic permissions while the image is inactive.
> 
> What are the actual problems that we're solving with this and the
> .bdrv_invalidate_cache/.bdrv_inactivate callbacks?

To handle locks in "-incoming" case. Removing this check will result in an
early locking error:

    (gdb) bt
    #0  0x0000555555ba40e7 in raw_check_lock_bytes 
    #1  0x0000555555ba4351 in raw_handle_perm_lock 
        at /stor/work/qemu/block/file-posix.c:729
    #2  0x0000555555ba6984 in raw_check_perm 
    #3  0x0000555555b4b3b2 in bdrv_check_perm 
        at /stor/work/qemu/block.c:1480
    #4  0x0000555555b4baae in bdrv_check_update_perm 
        at /stor/work/qemu/block.c:1665
    #5  0x0000555555b4c0da in bdrv_root_attach_child 
    #6  0x0000555555b4c299 in bdrv_attach_child 
    #7  0x0000555555b4cbc1 in bdrv_open_child 
    #8  0x0000555555b74703 in qcow2_open 
    #9  0x0000555555b4a362 in bdrv_open_driver 
    #10 0x0000555555b4acc2 in bdrv_open_common 
    #11 0x0000555555b4d592 in bdrv_open_inherit 
    #12 0x0000555555b4d9a9 in bdrv_open 
        at /stor/work/qemu/block.c:2538
    #13 0x0000555555b9c185 in blk_new_open 
        at /stor/work/qemu/block/block-backend.c:212
    #14 0x00005555558dcc0c in blockdev_init 
    #15 0x00005555558ddcee in drive_new 
    #16 0x00005555558ed6fd in drive_init_func 
    #17 0x0000555555c58fa0 in qemu_opts_foreach 
        at /stor/work/qemu/util/qemu-option.c:1114
    #18 0x00005555558f620f in main 

> 
> > +    assert(s->lock_fd > 0);
> > +
> > +    switch (op) {
> > +    case RAW_PL_PREPARE:
> > +        ret = raw_apply_lock_bytes(s, s->perm | new_perm,
> > +                                   ~s->shared_perm | ~new_shared,
> > +                                   false, errp);
> > +        if (!ret) {
> > +            ret = raw_check_lock_bytes(s, new_perm, new_shared, errp);
> > +            if (!ret) {
> > +                return 0;
> > +            }
> > +        }
> > +        op = RAW_PL_ABORT;
> 
> op isn't used after this place, I wouldn't be surprised if some compiler
> or static analyser complained about it.

I just don't want to surprise the "case RAW_PL_ABORT:" code. :)

> 
> > +        /* fall through to unlock bytes. */
> 
> Good, this undoes whatever raw_apply_lock_bytes() already has done.
> 
> > +    case RAW_PL_ABORT:
> > +        raw_apply_lock_bytes(s, s->perm, ~s->shared_perm, true, &local_err);
> > +        if (local_err) {
> > +            /* Theoretically the above call only unlocks bytes and it cannot
> > +             * fail. Something weird happened, report it.
> > +             */
> > +            error_report_err(local_err);
> > +        }
> > +        break;
> > +    case RAW_PL_COMMIT:
> > +        raw_apply_lock_bytes(s, new_perm, ~new_shared, true, &local_err);
> > +        if (local_err) {
> > +            /* Theoretically the above call only unlocks bytes and it cannot
> > +             * fail. Something weird happened, report it.
> > +             */
> > +            error_report_err(local_err);
> > +        }
> > +        break;
> > +    }
> > +    return ret;
> > +}
> > +
> >  static int raw_reopen_prepare(BDRVReopenState *state,
> >                                BlockReopenQueue *queue, Error **errp)
> >  {
> > @@ -549,6 +732,8 @@ static int raw_reopen_prepare(BDRVReopenState *state,
> >      BDRVRawReopenState *rs;
> >      int ret = 0;
> >      Error *local_err = NULL;
> > +    uint64_t clear_perms = state->flags & BDRV_O_RDWR ? 0 :
> > +        BLK_PERM_WRITE | BLK_PERM_RESIZE | BLK_PERM_WRITE_UNCHANGED;
> >  
> >      assert(state != NULL);
> >      assert(state->bs != NULL);
> > @@ -613,13 +798,22 @@ static int raw_reopen_prepare(BDRVReopenState *state,
> >      if (rs->fd != -1) {
> >          raw_probe_alignment(state->bs, rs->fd, &local_err);
> >          if (local_err) {
> > -            qemu_close(rs->fd);
> > -            rs->fd = -1;
> >              error_propagate(errp, local_err);
> >              ret = -EINVAL;
> > +            goto fail;
> >          }
> >      }
> >  
> > +    ret = raw_handle_perm_lock(state->bs, RAW_PL_PREPARE,
> > +                               s->perm & ~clear_perms,
> > +                               s->shared_perm, errp);
> 
> Is this a workaround for bdrv_can_set_read_only() not checking whether
> there are still writers attached? Because I think in theory, we should
> be able to assert() that clear_perms are already clear.

You are right, it seems these reopen hunks are superfluous. I will drop them.

> 
> > +    if (ret) {
> > +        goto fail;
> > +    }
> > +    return 0;
> > +fail:
> > +    qemu_close(rs->fd);
> > +    rs->fd = -1;
> >      return ret;
> >  }
> >  
> > @@ -627,6 +821,8 @@ static void raw_reopen_commit(BDRVReopenState *state)
> >  {
> >      BDRVRawReopenState *rs = state->opaque;
> >      BDRVRawState *s = state->bs->opaque;
> > +    uint64_t clear_perms = state->flags & BDRV_O_RDWR ? 0 :
> > +        BLK_PERM_WRITE | BLK_PERM_RESIZE | BLK_PERM_WRITE_UNCHANGED;
> >  
> >      s->open_flags = rs->open_flags;
> >  
> > @@ -635,12 +831,17 @@ static void raw_reopen_commit(BDRVReopenState *state)
> >  
> >      g_free(state->opaque);
> >      state->opaque = NULL;
> > +    raw_handle_perm_lock(state->bs, RAW_PL_COMMIT, s->perm & ~clear_perms,
> > +                         s->shared_perm, NULL);
> 
> Shouldn't we update s->perm here? Or probably move the update into
> raw_handle_perm_lock(). Or even more probably, as said above, replace
> the permission handling in raw_reopen_* by checking permissions in the
> generic block layer beforehand.
> 
> >  }
> >  
> >  
> >  static void raw_reopen_abort(BDRVReopenState *state)
> >  {
> > +    BDRVRawState *s = state->bs->opaque;
> >      BDRVRawReopenState *rs = state->opaque;
> > +    uint64_t clear_perms = state->flags & BDRV_O_RDWR ? 0 :
> > +        BLK_PERM_WRITE | BLK_PERM_RESIZE | BLK_PERM_WRITE_UNCHANGED;
> >  
> >       /* nothing to do if NULL, we didn't get far enough */
> >      if (rs == NULL) {
> > @@ -653,6 +854,8 @@ static void raw_reopen_abort(BDRVReopenState *state)
> >      }
> >      g_free(state->opaque);
> >      state->opaque = NULL;
> > +    raw_handle_perm_lock(state->bs, RAW_PL_ABORT, s->perm & ~clear_perms,
> > +                         s->shared_perm, NULL);
> >  }
> >  
> >  static int hdev_get_max_transfer_length(BlockDriverState *bs, int fd)
> > @@ -1410,6 +1613,10 @@ static void raw_close(BlockDriverState *bs)
> >          qemu_close(s->fd);
> >          s->fd = -1;
> >      }
> > +    if (s->lock_fd >= 0) {
> > +        qemu_close(s->lock_fd);
> > +        s->lock_fd = -1;
> > +    }
> >  }
> >  
> >  static int raw_truncate(BlockDriverState *bs, int64_t offset)
> > @@ -1947,6 +2154,56 @@ static QemuOptsList raw_create_opts = {
> >      }
> >  };
> >  
> > +static int raw_check_perm(BlockDriverState *bs, uint64_t perm, uint64_t shared,
> > +                          Error **errp)
> > +{
> > +    return raw_handle_perm_lock(bs, RAW_PL_PREPARE, perm, shared, errp);
> > +}
> > +
> > +static void raw_set_perm(BlockDriverState *bs, uint64_t perm, uint64_t shared)
> > +{
> > +    BDRVRawState *s = bs->opaque;
> > +    raw_handle_perm_lock(bs, RAW_PL_COMMIT, perm, shared, NULL);
> > +    s->perm = perm;
> > +    s->shared_perm = shared;
> > +}
> > +
> > +static void raw_abort_perm_update(BlockDriverState *bs)
> > +{
> > +    BDRVRawState *s = bs->opaque;
> > +
> > +    raw_handle_perm_lock(bs, RAW_PL_ABORT, s->perm, s->shared_perm, NULL);
> 
> The parameters are supposed to be the new permissions, which are
> obviously ignored in the case of RAW_PL_ABORT. Would passing 0 for
> both be more obvious?

OK, I'll change that.

> 
> > +}
> > +
> > +static int raw_inactivate(BlockDriverState *bs)
> > +{
> > +    int ret;
> > +    uint64_t perm = 0;
> > +    uint64_t shared = BLK_PERM_ALL;
> > +
> > +    ret = raw_handle_perm_lock(bs, RAW_PL_PREPARE, perm, shared, NULL);
> > +    if (ret) {
> > +        return ret;
> > +    }
> > +    raw_handle_perm_lock(bs, RAW_PL_COMMIT, perm, shared, NULL);
> > +    return 0;
> > +}
> > +
> > +
> > +static void raw_invalidate_cache(BlockDriverState *bs, Error **errp)
> > +{
> > +    BDRVRawState *s = bs->opaque;
> > +    int ret;
> > +
> > +    assert(!(bdrv_get_flags(bs) & BDRV_O_INACTIVE));
> > +    ret = raw_handle_perm_lock(bs, RAW_PL_PREPARE, s->perm, s->shared_perm,
> > +                               errp);
> > +    if (ret) {
> > +        return;
> > +    }
> > +    raw_handle_perm_lock(bs, RAW_PL_COMMIT, s->perm, s->shared_perm, NULL);
> > +}
> 
> Looks okay if we really need BDRV_O_INACTIVE handling in file-posix.
> 
> Kevin

Fam

Re: [Qemu-devel] [PATCH v15 20/21] file-posix: Add image locking to perm operations

[Kevin Wolf]

Am 26.04.2017 um 05:34 hat Fam Zheng geschrieben:
> This extends the permission bits of op blocker API to external using
> Linux OFD locks.
> 
> Each permission in @perm and @shared_perm is represented by a locked
> byte in the image file.  Requesting a permission in @perm is translated
> to a shared lock of the corresponding byte; rejecting to share the same
> permission is translated to a shared lock of a separate byte. With that,
> we use 2x number of bytes of distinct permission types.
> 
> virtlockd in libvirt locks the first byte, so we do locking from a
> higher offset.
> 
> Suggested-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Fam Zheng <famz@redhat.com>

>  BlockDriver bdrv_file = {
>      .format_name = "file",
>      .protocol_name = "file",
> @@ -1977,7 +2234,11 @@ BlockDriver bdrv_file = {
>      .bdrv_get_info = raw_get_info,
>      .bdrv_get_allocated_file_size
>                          = raw_get_allocated_file_size,
> -
> +    .bdrv_inactivate = raw_inactivate,
> +    .bdrv_invalidate_cache = raw_invalidate_cache,
> +    .bdrv_check_perm = raw_check_perm,
> +    .bdrv_set_perm   = raw_set_perm,
> +    .bdrv_abort_perm_update = raw_abort_perm_update,
>      .create_opts = &raw_create_opts,
>  };

By the way, is it intentional that we apply locking only to bdrv_file,
but not to bdrv_host_device or bdrv_host_cdrom?

Kevin

Re: [Qemu-devel] [PATCH v15 20/21] file-posix: Add image locking to perm operations

[Fam Zheng]

On Fri, 04/28 15:45, Kevin Wolf wrote:
> Am 26.04.2017 um 05:34 hat Fam Zheng geschrieben:
> > This extends the permission bits of op blocker API to external using
> > Linux OFD locks.
> > 
> > Each permission in @perm and @shared_perm is represented by a locked
> > byte in the image file.  Requesting a permission in @perm is translated
> > to a shared lock of the corresponding byte; rejecting to share the same
> > permission is translated to a shared lock of a separate byte. With that,
> > we use 2x number of bytes of distinct permission types.
> > 
> > virtlockd in libvirt locks the first byte, so we do locking from a
> > higher offset.
> > 
> > Suggested-by: Kevin Wolf <kwolf@redhat.com>
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> 
> >  BlockDriver bdrv_file = {
> >      .format_name = "file",
> >      .protocol_name = "file",
> > @@ -1977,7 +2234,11 @@ BlockDriver bdrv_file = {
> >      .bdrv_get_info = raw_get_info,
> >      .bdrv_get_allocated_file_size
> >                          = raw_get_allocated_file_size,
> > -
> > +    .bdrv_inactivate = raw_inactivate,
> > +    .bdrv_invalidate_cache = raw_invalidate_cache,
> > +    .bdrv_check_perm = raw_check_perm,
> > +    .bdrv_set_perm   = raw_set_perm,
> > +    .bdrv_abort_perm_update = raw_abort_perm_update,
> >      .create_opts = &raw_create_opts,
> >  };
> 
> By the way, is it intentional that we apply locking only to bdrv_file,
> but not to bdrv_host_device or bdrv_host_cdrom?

Good question.

Though CDROM is not very interesting, I am not sure about bdrv_host_device:

1) On the one hand, a host device can contain a qcow2 image so maybe locking is
useful.  Another reason to lock is that they share the same QAPI option,
BlockdevOptionsFile. That last reason is, it should be very easy to add it.

2) On the other hand, unlike files, host devices get pretty high chances in
being accesses by multiple readers/writers, outside of QEMU, such as partition
detection, mount, fsck, etc.

What is your opinion?

Fam

Re: [Qemu-devel] [PATCH v15 20/21] file-posix: Add image locking to perm operations

[Kevin Wolf]

Am 28.04.2017 um 17:30 hat Fam Zheng geschrieben:
> On Fri, 04/28 15:45, Kevin Wolf wrote:
> > Am 26.04.2017 um 05:34 hat Fam Zheng geschrieben:
> > > This extends the permission bits of op blocker API to external using
> > > Linux OFD locks.
> > > 
> > > Each permission in @perm and @shared_perm is represented by a locked
> > > byte in the image file.  Requesting a permission in @perm is translated
> > > to a shared lock of the corresponding byte; rejecting to share the same
> > > permission is translated to a shared lock of a separate byte. With that,
> > > we use 2x number of bytes of distinct permission types.
> > > 
> > > virtlockd in libvirt locks the first byte, so we do locking from a
> > > higher offset.
> > > 
> > > Suggested-by: Kevin Wolf <kwolf@redhat.com>
> > > Signed-off-by: Fam Zheng <famz@redhat.com>
> > 
> > >  BlockDriver bdrv_file = {
> > >      .format_name = "file",
> > >      .protocol_name = "file",
> > > @@ -1977,7 +2234,11 @@ BlockDriver bdrv_file = {
> > >      .bdrv_get_info = raw_get_info,
> > >      .bdrv_get_allocated_file_size
> > >                          = raw_get_allocated_file_size,
> > > -
> > > +    .bdrv_inactivate = raw_inactivate,
> > > +    .bdrv_invalidate_cache = raw_invalidate_cache,
> > > +    .bdrv_check_perm = raw_check_perm,
> > > +    .bdrv_set_perm   = raw_set_perm,
> > > +    .bdrv_abort_perm_update = raw_abort_perm_update,
> > >      .create_opts = &raw_create_opts,
> > >  };
> > 
> > By the way, is it intentional that we apply locking only to bdrv_file,
> > but not to bdrv_host_device or bdrv_host_cdrom?
> 
> Good question.
> 
> Though CDROM is not very interesting, I am not sure about bdrv_host_device:
> 
> 1) On the one hand, a host device can contain a qcow2 image so maybe locking is
> useful.  Another reason to lock is that they share the same QAPI option,
> BlockdevOptionsFile. That last reason is, it should be very easy to add it.
> 
> 2) On the other hand, unlike files, host devices get pretty high chances in
> being accesses by multiple readers/writers, outside of QEMU, such as partition
> detection, mount, fsck, etc.
> 
> What is your opinion?

I would add it, if nothing else to be consistent with regular files. You
don't even need qcow2 on a block device for it to be useful, even for
raw images the guest may not be able to tolerate a second writer (in
this case, share-rw of the qdev device will directly control the locking
mode).

As for 2), I don't think these other users are a problem because we're
only taking shared locks. We'll prevent other users from taking an
exclusive lock, but that's exactly right because it's true that we're
using the image.

Kevin

[Qemu-devel] [PATCH v15 21/21] qemu-iotests: Add test case 153 for image locking

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 tests/qemu-iotests/153     | 220 +++++++++++++++++++++++++
 tests/qemu-iotests/153.out | 390 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 611 insertions(+)
 create mode 100755 tests/qemu-iotests/153
 create mode 100644 tests/qemu-iotests/153.out

diff --git a/tests/qemu-iotests/153 b/tests/qemu-iotests/153
new file mode 100755
index 0000000..7501efa
--- /dev/null
+++ b/tests/qemu-iotests/153
@@ -0,0 +1,220 @@
+#!/bin/bash
+#
+# Test image locking
+#
+# Copyright 2016, 2017 Red Hat, Inc.
+#
+# 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=famz@redhat.com
+
+seq="$(basename $0)"
+echo "QA output created by $seq"
+
+here="$PWD"
+tmp=/tmp/$$
+status=1	# failure is the default!
+
+_cleanup()
+{
+    _cleanup_test_img
+    rm -f "${TEST_IMG}.base"
+    rm -f "${TEST_IMG}.convert"
+    rm -f "${TEST_IMG}.a"
+    rm -f "${TEST_IMG}.b"
+    rm -f "${TEST_IMG}.lnk"
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+. ./common.qemu
+
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+
+size=32M
+
+_run_cmd()
+{
+    echo
+    (echo "$@"; "$@" 2>&1 1>/dev/null) | _filter_testdir
+}
+
+function _do_run_qemu()
+{
+    (
+        if ! test -t 0; then
+            while read cmd; do
+                echo $cmd
+            done
+        fi
+        echo quit
+    ) | $QEMU -nographic -monitor stdio -serial none "$@" 1>/dev/null
+}
+
+function _run_qemu_with_images()
+{
+    _do_run_qemu \
+        $(for i in $@; do echo "-drive if=none,file=$i"; done) 2>&1 \
+        | _filter_testdir | _filter_qemu
+}
+
+echo "== readonly=off,force-share=on should be rejected =="
+_run_qemu_with_images null-co://,readonly=off,force-share=on
+
+for opts1 in "" "read-only=on" "read-only=on,force-share=on"; do
+    echo
+    echo "== Creating base image =="
+    TEST_IMG="${TEST_IMG}.base" _make_test_img $size
+
+    echo
+    echo "== Creating test image =="
+    $QEMU_IMG create -f $IMGFMT "${TEST_IMG}" -b ${TEST_IMG}.base | _filter_img_create
+
+    echo
+    echo "== Launching QEMU, opts: '$opts1' =="
+    _launch_qemu -drive file="${TEST_IMG}",if=none,$opts1
+    h=$QEMU_HANDLE
+
+    for opts2 in "" "read-only=on" "read-only=on,force-share=on"; do
+        echo
+        echo "== Launching another QEMU, opts: '$opts2' =="
+        echo "quit" | \
+            $QEMU -nographic -monitor stdio \
+            -drive file="${TEST_IMG}",if=none,$opts2 2>&1 1>/dev/null | \
+            _filter_testdir | _filter_qemu
+    done
+
+    for L in "" "-U"; do
+
+        echo
+        echo "== Running utility commands $L =="
+        _run_cmd $QEMU_IO $L -c "read 0 512" "${TEST_IMG}"
+        _run_cmd $QEMU_IO $L -r -c "read 0 512" "${TEST_IMG}"
+        _run_cmd $QEMU_IO -c "open $L ${TEST_IMG}" -c "read 0 512"
+        _run_cmd $QEMU_IO -c "open -r $L ${TEST_IMG}" -c "read 0 512"
+        _run_cmd $QEMU_IMG info        $L "${TEST_IMG}"
+        _run_cmd $QEMU_IMG check       $L "${TEST_IMG}"
+        _run_cmd $QEMU_IMG compare     $L "${TEST_IMG}" "${TEST_IMG}"
+        _run_cmd $QEMU_IMG map         $L "${TEST_IMG}"
+        _run_cmd $QEMU_IMG amend -o "" $L "${TEST_IMG}"
+        _run_cmd $QEMU_IMG commit      $L "${TEST_IMG}"
+        _run_cmd $QEMU_IMG resize      $L "${TEST_IMG}" $size
+        _run_cmd $QEMU_IMG rebase      $L "${TEST_IMG}" -b "${TEST_IMG}.base"
+        _run_cmd $QEMU_IMG snapshot -l $L "${TEST_IMG}"
+        _run_cmd $QEMU_IMG convert     $L "${TEST_IMG}" "${TEST_IMG}.convert"
+        _run_cmd $QEMU_IMG dd          $L if="${TEST_IMG}" of="${TEST_IMG}.convert" bs=512 count=1
+        _run_cmd $QEMU_IMG bench       $L -c 1 "${TEST_IMG}"
+        _run_cmd $QEMU_IMG bench       $L -w -c 1 "${TEST_IMG}"
+    done
+    _send_qemu_cmd $h "{ 'execute': 'quit', }" ""
+    echo
+    echo "Round done"
+    _cleanup_qemu
+done
+
+for opt1 in $test_opts; do
+    for opt2 in $test_opts; do
+        echo
+        echo "== Two devices with the same image ($opt1 - $opt2) =="
+        _run_qemu_with_images "${TEST_IMG},$opt1" "${TEST_IMG},$opt2"
+    done
+done
+
+echo "== Creating ${TEST_IMG}.[abc] =="
+(
+    $QEMU_IMG create -f qcow2 "${TEST_IMG}.a" -b "${TEST_IMG}"
+    $QEMU_IMG create -f qcow2 "${TEST_IMG}.b" -b "${TEST_IMG}"
+    $QEMU_IMG create -f qcow2 "${TEST_IMG}.c" -b "${TEST_IMG}.b"
+) | _filter_img_create
+
+echo
+echo "== Two devices sharing the same file in backing chain =="
+_run_qemu_with_images "${TEST_IMG}.a" "${TEST_IMG}.b"
+_run_qemu_with_images "${TEST_IMG}.a" "${TEST_IMG}.c"
+
+echo
+echo "== Backing image also as an active device =="
+_run_qemu_with_images "${TEST_IMG}.a" "${TEST_IMG}"
+
+echo
+echo "== Backing image also as an active device (ro) =="
+_run_qemu_with_images "${TEST_IMG}.a" "${TEST_IMG},readonly=on"
+
+echo
+echo "== Symbolic link =="
+rm -f "${TEST_IMG}.lnk" &>/dev/null
+ln -s ${TEST_IMG} "${TEST_IMG}.lnk" || echo "Failed to create link"
+_run_qemu_with_images "${TEST_IMG}.lnk" "${TEST_IMG}"
+
+echo
+echo "== Closing an image should unlock it =="
+_launch_qemu
+
+_send_qemu_cmd $QEMU_HANDLE \
+    "{ 'execute': 'qmp_capabilities' }" \
+    'return'
+
+echo "Adding drive"
+_send_qemu_cmd $QEMU_HANDLE \
+    "{ 'execute': 'human-monitor-command',
+       'arguments': { 'command-line': 'drive_add 0 if=none,id=d0,file=${TEST_IMG}' } }" \
+    ""
+
+_run_cmd $QEMU_IO "${TEST_IMG}" -c 'write 0 512'
+
+echo "Closing drive"
+_send_qemu_cmd $QEMU_HANDLE \
+    "{ 'execute': 'human-monitor-command',
+       'arguments': { 'command-line': 'drive_del d0' } }" \
+    ""
+
+_run_cmd $QEMU_IO "${TEST_IMG}" -c 'write 0 512'
+
+echo "Adding two and closing one"
+for d in d0 d1; do
+    _send_qemu_cmd $QEMU_HANDLE \
+        "{ 'execute': 'human-monitor-command',
+           'arguments': { 'command-line': 'drive_add 0 if=none,id=$d,file=${TEST_IMG},readonly=on' } }" \
+        ""
+done
+
+_run_cmd $QEMU_IMG info "${TEST_IMG}"
+
+_send_qemu_cmd $QEMU_HANDLE \
+    "{ 'execute': 'human-monitor-command',
+       'arguments': { 'command-line': 'drive_del d0' } }" \
+    ""
+
+_run_cmd $QEMU_IO "${TEST_IMG}" -c 'write 0 512'
+
+echo "Closing the other"
+_send_qemu_cmd $QEMU_HANDLE \
+    "{ 'execute': 'human-monitor-command',
+       'arguments': { 'command-line': 'drive_del d1' } }" \
+    ""
+
+_run_cmd $QEMU_IO "${TEST_IMG}" -c 'write 0 512'
+
+_cleanup_qemu
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/153.out b/tests/qemu-iotests/153.out
new file mode 100644
index 0000000..3956de6
--- /dev/null
+++ b/tests/qemu-iotests/153.out
@@ -0,0 +1,390 @@
+QA output created by 153
+== readonly=off,force-share=on should be rejected ==
+QEMU_PROG: -drive if=none,file=null-co://,readonly=off,force-share=on: force-share=on can only be used with read-only images
+
+== Creating base image ==
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=33554432
+
+== Creating test image ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=33554432 backing_file=TEST_DIR/t.IMGFMT.base
+
+== Launching QEMU, opts: '' ==
+
+== Launching another QEMU, opts: '' ==
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,if=none,: Failed to check byte 201 for "write" permission
+Is another process using the image?
+
+== Launching another QEMU, opts: 'read-only=on' ==
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,if=none,read-only=on: Failed to check byte 101 for shared "write" permission
+Is another process using the image?
+
+== Launching another QEMU, opts: 'read-only=on,force-share=on' ==
+
+== Running utility commands  ==
+
+_qemu_io_wrapper -c read 0 512 TEST_DIR/t.qcow2
+can't open device TEST_DIR/t.qcow2: Failed to check byte 201 for "write" permission
+Is another process using the image?
+
+_qemu_io_wrapper -r -c read 0 512 TEST_DIR/t.qcow2
+can't open device TEST_DIR/t.qcow2: Failed to check byte 101 for shared "write" permission
+Is another process using the image?
+
+_qemu_io_wrapper -c open  TEST_DIR/t.qcow2 -c read 0 512
+can't open device TEST_DIR/t.qcow2: Failed to check byte 201 for "write" permission
+Is another process using the image?
+
+_qemu_io_wrapper -c open -r  TEST_DIR/t.qcow2 -c read 0 512
+can't open device TEST_DIR/t.qcow2: Failed to check byte 101 for shared "write" permission
+Is another process using the image?
+
+_qemu_img_wrapper info TEST_DIR/t.qcow2
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to check byte 101 for shared "write" permission
+Is another process using the image?
+
+_qemu_img_wrapper check TEST_DIR/t.qcow2
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to check byte 101 for shared "write" permission
+Is another process using the image?
+
+_qemu_img_wrapper compare TEST_DIR/t.qcow2 TEST_DIR/t.qcow2
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to check byte 101 for shared "write" permission
+Is another process using the image?
+
+_qemu_img_wrapper map TEST_DIR/t.qcow2
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to check byte 101 for shared "write" permission
+Is another process using the image?
+
+_qemu_img_wrapper amend -o  TEST_DIR/t.qcow2
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to check byte 201 for "write" permission
+Is another process using the image?
+
+_qemu_img_wrapper commit TEST_DIR/t.qcow2
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to check byte 201 for "write" permission
+Is another process using the image?
+
+_qemu_img_wrapper resize TEST_DIR/t.qcow2 32M
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to check byte 201 for "write" permission
+Is another process using the image?
+
+_qemu_img_wrapper rebase TEST_DIR/t.qcow2 -b TEST_DIR/t.qcow2.base
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to check byte 201 for "write" permission
+Is another process using the image?
+
+_qemu_img_wrapper snapshot -l TEST_DIR/t.qcow2
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to check byte 101 for shared "write" permission
+Is another process using the image?
+
+_qemu_img_wrapper convert TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.convert
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to check byte 101 for shared "write" permission
+Is another process using the image?
+
+_qemu_img_wrapper dd if=TEST_DIR/t.qcow2 of=TEST_DIR/t.qcow2.convert bs=512 count=1
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to check byte 101 for shared "write" permission
+Is another process using the image?
+
+_qemu_img_wrapper bench -c 1 TEST_DIR/t.qcow2
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to check byte 101 for shared "write" permission
+Is another process using the image?
+
+_qemu_img_wrapper bench -w -c 1 TEST_DIR/t.qcow2
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to check byte 201 for "write" permission
+Is another process using the image?
+
+== Running utility commands -U ==
+
+_qemu_io_wrapper -U -c read 0 512 TEST_DIR/t.qcow2
+can't open device TEST_DIR/t.qcow2: force-share=on can only be used with read-only images
+
+_qemu_io_wrapper -U -r -c read 0 512 TEST_DIR/t.qcow2
+
+_qemu_io_wrapper -c open -U TEST_DIR/t.qcow2 -c read 0 512
+can't open device TEST_DIR/t.qcow2: force-share=on can only be used with read-only images
+
+_qemu_io_wrapper -c open -r -U TEST_DIR/t.qcow2 -c read 0 512
+
+_qemu_img_wrapper info -U TEST_DIR/t.qcow2
+
+_qemu_img_wrapper check -U TEST_DIR/t.qcow2
+
+_qemu_img_wrapper compare -U TEST_DIR/t.qcow2 TEST_DIR/t.qcow2
+
+_qemu_img_wrapper map -U TEST_DIR/t.qcow2
+
+_qemu_img_wrapper amend -o  -U TEST_DIR/t.qcow2
+qemu-img: unrecognized option '-U'
+Try 'qemu-img --help' for more information
+
+_qemu_img_wrapper commit -U TEST_DIR/t.qcow2
+qemu-img: unrecognized option '-U'
+Try 'qemu-img --help' for more information
+
+_qemu_img_wrapper resize -U TEST_DIR/t.qcow2 32M
+qemu-img: unrecognized option '-U'
+Try 'qemu-img --help' for more information
+
+_qemu_img_wrapper rebase -U TEST_DIR/t.qcow2 -b TEST_DIR/t.qcow2.base
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to check byte 201 for "write" permission
+Is another process using the image?
+
+_qemu_img_wrapper snapshot -l -U TEST_DIR/t.qcow2
+
+_qemu_img_wrapper convert -U TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.convert
+
+_qemu_img_wrapper dd -U if=TEST_DIR/t.qcow2 of=TEST_DIR/t.qcow2.convert bs=512 count=1
+
+_qemu_img_wrapper bench -U -c 1 TEST_DIR/t.qcow2
+
+_qemu_img_wrapper bench -U -w -c 1 TEST_DIR/t.qcow2
+qemu-img: Could not open 'TEST_DIR/t.qcow2': force-share=on can only be used with read-only images
+
+Round done
+
+== Creating base image ==
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=33554432
+
+== Creating test image ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=33554432 backing_file=TEST_DIR/t.IMGFMT.base
+
+== Launching QEMU, opts: 'read-only=on' ==
+
+== Launching another QEMU, opts: '' ==
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,if=none,: Failed to check byte 201 for "write" permission
+Is another process using the image?
+
+== Launching another QEMU, opts: 'read-only=on' ==
+
+== Launching another QEMU, opts: 'read-only=on,force-share=on' ==
+
+== Running utility commands  ==
+
+_qemu_io_wrapper -c read 0 512 TEST_DIR/t.qcow2
+can't open device TEST_DIR/t.qcow2: Failed to check byte 201 for "write" permission
+Is another process using the image?
+
+_qemu_io_wrapper -r -c read 0 512 TEST_DIR/t.qcow2
+
+_qemu_io_wrapper -c open  TEST_DIR/t.qcow2 -c read 0 512
+can't open device TEST_DIR/t.qcow2: Failed to check byte 201 for "write" permission
+Is another process using the image?
+
+_qemu_io_wrapper -c open -r  TEST_DIR/t.qcow2 -c read 0 512
+
+_qemu_img_wrapper info TEST_DIR/t.qcow2
+
+_qemu_img_wrapper check TEST_DIR/t.qcow2
+
+_qemu_img_wrapper compare TEST_DIR/t.qcow2 TEST_DIR/t.qcow2
+
+_qemu_img_wrapper map TEST_DIR/t.qcow2
+
+_qemu_img_wrapper amend -o  TEST_DIR/t.qcow2
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to check byte 201 for "write" permission
+Is another process using the image?
+
+_qemu_img_wrapper commit TEST_DIR/t.qcow2
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to check byte 201 for "write" permission
+Is another process using the image?
+
+_qemu_img_wrapper resize TEST_DIR/t.qcow2 32M
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to check byte 201 for "write" permission
+Is another process using the image?
+
+_qemu_img_wrapper rebase TEST_DIR/t.qcow2 -b TEST_DIR/t.qcow2.base
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to check byte 201 for "write" permission
+Is another process using the image?
+
+_qemu_img_wrapper snapshot -l TEST_DIR/t.qcow2
+
+_qemu_img_wrapper convert TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.convert
+
+_qemu_img_wrapper dd if=TEST_DIR/t.qcow2 of=TEST_DIR/t.qcow2.convert bs=512 count=1
+
+_qemu_img_wrapper bench -c 1 TEST_DIR/t.qcow2
+
+_qemu_img_wrapper bench -w -c 1 TEST_DIR/t.qcow2
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to check byte 201 for "write" permission
+Is another process using the image?
+
+== Running utility commands -U ==
+
+_qemu_io_wrapper -U -c read 0 512 TEST_DIR/t.qcow2
+can't open device TEST_DIR/t.qcow2: force-share=on can only be used with read-only images
+
+_qemu_io_wrapper -U -r -c read 0 512 TEST_DIR/t.qcow2
+
+_qemu_io_wrapper -c open -U TEST_DIR/t.qcow2 -c read 0 512
+can't open device TEST_DIR/t.qcow2: force-share=on can only be used with read-only images
+
+_qemu_io_wrapper -c open -r -U TEST_DIR/t.qcow2 -c read 0 512
+
+_qemu_img_wrapper info -U TEST_DIR/t.qcow2
+
+_qemu_img_wrapper check -U TEST_DIR/t.qcow2
+
+_qemu_img_wrapper compare -U TEST_DIR/t.qcow2 TEST_DIR/t.qcow2
+
+_qemu_img_wrapper map -U TEST_DIR/t.qcow2
+
+_qemu_img_wrapper amend -o  -U TEST_DIR/t.qcow2
+qemu-img: unrecognized option '-U'
+Try 'qemu-img --help' for more information
+
+_qemu_img_wrapper commit -U TEST_DIR/t.qcow2
+qemu-img: unrecognized option '-U'
+Try 'qemu-img --help' for more information
+
+_qemu_img_wrapper resize -U TEST_DIR/t.qcow2 32M
+qemu-img: unrecognized option '-U'
+Try 'qemu-img --help' for more information
+
+_qemu_img_wrapper rebase -U TEST_DIR/t.qcow2 -b TEST_DIR/t.qcow2.base
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to check byte 201 for "write" permission
+Is another process using the image?
+
+_qemu_img_wrapper snapshot -l -U TEST_DIR/t.qcow2
+
+_qemu_img_wrapper convert -U TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.convert
+
+_qemu_img_wrapper dd -U if=TEST_DIR/t.qcow2 of=TEST_DIR/t.qcow2.convert bs=512 count=1
+
+_qemu_img_wrapper bench -U -c 1 TEST_DIR/t.qcow2
+
+_qemu_img_wrapper bench -U -w -c 1 TEST_DIR/t.qcow2
+qemu-img: Could not open 'TEST_DIR/t.qcow2': force-share=on can only be used with read-only images
+
+Round done
+
+== Creating base image ==
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=33554432
+
+== Creating test image ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=33554432 backing_file=TEST_DIR/t.IMGFMT.base
+
+== Launching QEMU, opts: 'read-only=on,force-share=on' ==
+
+== Launching another QEMU, opts: '' ==
+
+== Launching another QEMU, opts: 'read-only=on' ==
+
+== Launching another QEMU, opts: 'read-only=on,force-share=on' ==
+
+== Running utility commands  ==
+
+_qemu_io_wrapper -c read 0 512 TEST_DIR/t.qcow2
+
+_qemu_io_wrapper -r -c read 0 512 TEST_DIR/t.qcow2
+
+_qemu_io_wrapper -c open  TEST_DIR/t.qcow2 -c read 0 512
+
+_qemu_io_wrapper -c open -r  TEST_DIR/t.qcow2 -c read 0 512
+
+_qemu_img_wrapper info TEST_DIR/t.qcow2
+
+_qemu_img_wrapper check TEST_DIR/t.qcow2
+
+_qemu_img_wrapper compare TEST_DIR/t.qcow2 TEST_DIR/t.qcow2
+
+_qemu_img_wrapper map TEST_DIR/t.qcow2
+
+_qemu_img_wrapper amend -o  TEST_DIR/t.qcow2
+
+_qemu_img_wrapper commit TEST_DIR/t.qcow2
+
+_qemu_img_wrapper resize TEST_DIR/t.qcow2 32M
+
+_qemu_img_wrapper rebase TEST_DIR/t.qcow2 -b TEST_DIR/t.qcow2.base
+
+_qemu_img_wrapper snapshot -l TEST_DIR/t.qcow2
+
+_qemu_img_wrapper convert TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.convert
+
+_qemu_img_wrapper dd if=TEST_DIR/t.qcow2 of=TEST_DIR/t.qcow2.convert bs=512 count=1
+
+_qemu_img_wrapper bench -c 1 TEST_DIR/t.qcow2
+
+_qemu_img_wrapper bench -w -c 1 TEST_DIR/t.qcow2
+
+== Running utility commands -U ==
+
+_qemu_io_wrapper -U -c read 0 512 TEST_DIR/t.qcow2
+can't open device TEST_DIR/t.qcow2: force-share=on can only be used with read-only images
+
+_qemu_io_wrapper -U -r -c read 0 512 TEST_DIR/t.qcow2
+
+_qemu_io_wrapper -c open -U TEST_DIR/t.qcow2 -c read 0 512
+can't open device TEST_DIR/t.qcow2: force-share=on can only be used with read-only images
+
+_qemu_io_wrapper -c open -r -U TEST_DIR/t.qcow2 -c read 0 512
+
+_qemu_img_wrapper info -U TEST_DIR/t.qcow2
+
+_qemu_img_wrapper check -U TEST_DIR/t.qcow2
+
+_qemu_img_wrapper compare -U TEST_DIR/t.qcow2 TEST_DIR/t.qcow2
+
+_qemu_img_wrapper map -U TEST_DIR/t.qcow2
+
+_qemu_img_wrapper amend -o  -U TEST_DIR/t.qcow2
+qemu-img: unrecognized option '-U'
+Try 'qemu-img --help' for more information
+
+_qemu_img_wrapper commit -U TEST_DIR/t.qcow2
+qemu-img: unrecognized option '-U'
+Try 'qemu-img --help' for more information
+
+_qemu_img_wrapper resize -U TEST_DIR/t.qcow2 32M
+qemu-img: unrecognized option '-U'
+Try 'qemu-img --help' for more information
+
+_qemu_img_wrapper rebase -U TEST_DIR/t.qcow2 -b TEST_DIR/t.qcow2.base
+
+_qemu_img_wrapper snapshot -l -U TEST_DIR/t.qcow2
+
+_qemu_img_wrapper convert -U TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.convert
+
+_qemu_img_wrapper dd -U if=TEST_DIR/t.qcow2 of=TEST_DIR/t.qcow2.convert bs=512 count=1
+
+_qemu_img_wrapper bench -U -c 1 TEST_DIR/t.qcow2
+
+_qemu_img_wrapper bench -U -w -c 1 TEST_DIR/t.qcow2
+qemu-img: Could not open 'TEST_DIR/t.qcow2': force-share=on can only be used with read-only images
+
+Round done
+== Creating /var/tmp/q/build/tests/qemu-iotests/scratch/t.qcow2.[abc] ==
+Formatting 'TEST_DIR/t.IMGFMT.a', fmt=IMGFMT size=33554432 backing_file=TEST_DIR/t.IMGFMT
+Formatting 'TEST_DIR/t.IMGFMT.b', fmt=IMGFMT size=33554432 backing_file=TEST_DIR/t.IMGFMT
+Formatting 'TEST_DIR/t.IMGFMT.c', fmt=IMGFMT size=33554432 backing_file=TEST_DIR/t.IMGFMT.b
+
+== Two devices sharing the same file in backing chain ==
+
+== Backing image also as an active device ==
+QEMU_PROG: -drive if=none,file=TEST_DIR/t.qcow2: Failed to check byte 201 for "write" permission
+Is another process using the image?
+
+== Backing image also as an active device (ro) ==
+
+== Symbolic link ==
+QEMU_PROG: -drive if=none,file=TEST_DIR/t.qcow2: Failed to check byte 201 for "write" permission
+Is another process using the image?
+
+== Closing an image should unlock it ==
+{"return": {}}
+Adding drive
+
+_qemu_io_wrapper TEST_DIR/t.qcow2 -c write 0 512
+can't open device TEST_DIR/t.qcow2: Failed to check byte 201 for "write" permission
+Is another process using the image?
+Closing drive
+
+_qemu_io_wrapper TEST_DIR/t.qcow2 -c write 0 512
+Adding two and closing one
+
+_qemu_img_wrapper info TEST_DIR/t.qcow2
+
+_qemu_io_wrapper TEST_DIR/t.qcow2 -c write 0 512
+can't open device TEST_DIR/t.qcow2: Failed to check byte 201 for "write" permission
+Is another process using the image?
+Closing the other
+
+_qemu_io_wrapper TEST_DIR/t.qcow2 -c write 0 512
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 893962d..c8ae60e 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -154,6 +154,7 @@
 149 rw auto sudo
 150 rw auto quick
 152 rw auto quick
+153 rw auto quick
 154 rw auto backing quick
 155 rw auto
 156 rw auto quick
-- 
2.9.3

Re: [Qemu-devel] [PATCH v15 21/21] qemu-iotests: Add test case 153 for image locking

[Fam Zheng]

On Wed, 04/26 11:34, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  tests/qemu-iotests/153     | 220 +++++++++++++++++++++++++
>  tests/qemu-iotests/153.out | 390 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/group   |   1 +
>  3 files changed, 611 insertions(+)
>  create mode 100755 tests/qemu-iotests/153
>  create mode 100644 tests/qemu-iotests/153.out
> 
> diff --git a/tests/qemu-iotests/153 b/tests/qemu-iotests/153
> +== Creating /var/tmp/q/build/tests/qemu-iotests/scratch/t.qcow2.[abc] ==

Forgot to filter test dir here.

Fam

Re: [Qemu-devel] [PATCH v15 21/21] qemu-iotests: Add test case 153 for image locking

[Kevin Wolf]

Am 26.04.2017 um 05:34 hat Fam Zheng geschrieben:
> Signed-off-by: Fam Zheng <famz@redhat.com>

I'll do the actual review of the test case later, but I already have a
question... Can we find a way to accept the expected lock losses on
systems without OFD? I'm using RHEL 7 as my main development OS, so I
wouldn't like having to deal with false positives there.

Hm... And actually, printing a warning will probably invalidate all the
other reference outputs, too. After all, every single invocation of
qemu/qemu-img/qemu-io will print a warning. Maybe that's a bit too much
even for manual users when they can't do anything about it (which is
unlike the format probing warning that you can get rid of by simply
changing your command line).

Kevin

Re: [Qemu-devel] [PATCH v15 21/21] qemu-iotests: Add test case 153 for image locking

[Fam Zheng]

On Wed, 04/26 16:49, Kevin Wolf wrote:
> Am 26.04.2017 um 05:34 hat Fam Zheng geschrieben:
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> 
> I'll do the actual review of the test case later, but I already have a
> question... Can we find a way to accept the expected lock losses on
> systems without OFD? I'm using RHEL 7 as my main development OS, so I
> wouldn't like having to deal with false positives there.

Split the lock loss cases to a separate case and _not_run? :)

> 
> Hm... And actually, printing a warning will probably invalidate all the
> other reference outputs, too. After all, every single invocation of
> qemu/qemu-img/qemu-io will print a warning. Maybe that's a bit too much
> even for manual users when they can't do anything about it (which is
> unlike the format probing warning that you can get rid of by simply
> changing your command line).

An idea: make locking=on/off/auto, and auto is on iff OFD is usable, otherwise
off. When user specifies locking=on but OFD is unusable, we print a warning.

Only in this on test case the warning will be printed on RHEL, and it's easy to
filter it out.

Fam

Re: [Qemu-devel] [PATCH v15 21/21] qemu-iotests: Add test case 153 for image locking

[Kevin Wolf]

Am 27.04.2017 um 03:32 hat Fam Zheng geschrieben:
> On Wed, 04/26 16:49, Kevin Wolf wrote:
> > Am 26.04.2017 um 05:34 hat Fam Zheng geschrieben:
> > > Signed-off-by: Fam Zheng <famz@redhat.com>
> > 
> > I'll do the actual review of the test case later, but I already have a
> > question... Can we find a way to accept the expected lock losses on
> > systems without OFD? I'm using RHEL 7 as my main development OS, so I
> > wouldn't like having to deal with false positives there.
> 
> Split the lock loss cases to a separate case and _not_run? :)

You still need to find out from a shell script whether OFD locks are
supported before you can call _not_run, but otherwise I'm fine with
this.

> > Hm... And actually, printing a warning will probably invalidate all the
> > other reference outputs, too. After all, every single invocation of
> > qemu/qemu-img/qemu-io will print a warning. Maybe that's a bit too much
> > even for manual users when they can't do anything about it (which is
> > unlike the format probing warning that you can get rid of by simply
> > changing your command line).
> 
> An idea: make locking=on/off/auto, and auto is on iff OFD is usable,
> otherwise off. When user specifies locking=on but OFD is unusable, we
> print a warning.
> 
> Only in this on test case the warning will be printed on RHEL, and
> it's easy to filter it out.

So POSIX locks only if you explicitly enable locking? It would be a pity
to do even less than we could theoretically do by default, but at least
the default wouldn't result in surprising behaviour by losing locks
randomly.

The argument for it would be that being consistently unsafe is better
than being unpredictably safe or unsafe. I always find this kind of
decision hard to make because I can appreciate both sides of the
argument.

Kevin

Re: [Qemu-devel] [PATCH v15 21/21] qemu-iotests: Add test case 153 for image locking

[Fam Zheng]

On Thu, 04/27 11:05, Kevin Wolf wrote:
> Am 27.04.2017 um 03:32 hat Fam Zheng geschrieben:
> > On Wed, 04/26 16:49, Kevin Wolf wrote:
> > > Am 26.04.2017 um 05:34 hat Fam Zheng geschrieben:
> > > > Signed-off-by: Fam Zheng <famz@redhat.com>
> > > 
> > > I'll do the actual review of the test case later, but I already have a
> > > question... Can we find a way to accept the expected lock losses on
> > > systems without OFD? I'm using RHEL 7 as my main development OS, so I
> > > wouldn't like having to deal with false positives there.
> > 
> > Split the lock loss cases to a separate case and _not_run? :)
> 
> You still need to find out from a shell script whether OFD locks are
> supported before you can call _not_run, but otherwise I'm fine with
> this.

OK. Shouldn't be rocket science. Let's try.

> 
> > > Hm... And actually, printing a warning will probably invalidate all the
> > > other reference outputs, too. After all, every single invocation of
> > > qemu/qemu-img/qemu-io will print a warning. Maybe that's a bit too much
> > > even for manual users when they can't do anything about it (which is
> > > unlike the format probing warning that you can get rid of by simply
> > > changing your command line).
> > 
> > An idea: make locking=on/off/auto, and auto is on iff OFD is usable,
> > otherwise off. When user specifies locking=on but OFD is unusable, we
> > print a warning.
> > 
> > Only in this on test case the warning will be printed on RHEL, and
> > it's easy to filter it out.
> 
> So POSIX locks only if you explicitly enable locking? It would be a pity
> to do even less than we could theoretically do by default, but at least
> the default wouldn't result in surprising behaviour by losing locks
> randomly.
> 
> The argument for it would be that being consistently unsafe is better
> than being unpredictably safe or unsafe. I always find this kind of
> decision hard to make because I can appreciate both sides of the
> argument.

For purists, consistency is perhaps more important; for pragmatists, some
protection is better than none. I figure it's up to the right of maintainers,
which you are.  :)

However I do think making noticable complaints about OFD missing (in the
documentation and/or the error messages) is of greater good, and is better than
silent yet incomplete safty.

We want to offer as much protection as possible to everyone but it's more
important when there is a qcow2 corruption report (I hope there isn't :) in the
future, we can be sure whether locking is in place or not, no iffy situation.
POSIX locking makes this harder, a warning message is helpful.

Fam