[Qemu-devel] [PATCH v5 00/27] block: Lock images when opening

[Qemu-devel] [PATCH v5 00/27] block: Lock images when opening

[Fam Zheng]

v5: - Change "lock-image=on/off" to "lock-mode=exclusive/shared/off".
      Default is "lock-mode=exclusive" to exclusively lock RW images and shared
      lock RO images; with lock-mode="shared", RW images are shared locked too;
      lock-mode=off turns off image locking completely.
    - Use F_OFD_SETLK fcntl so that close/dup on different fds are not a
      problem.
    - Update test cases.


Fam Zheng (27):
  block: Add flag bits for image locking
  qapi: Add lock-mode in blockdev-add options
  blockdev: Add and parse "lock-mode" option for image locking
  block: Introduce image file locking
  block: Add bdrv_image_locked
  block: Make bdrv_reopen_{commit, abort} private functions
  block: Handle image locking during reopen
  osdep: Add qemu_lock_fd and qemu_unlock_fd
  osdep: Introduce qemu_dup
  raw-posix: Use qemu_dup
  raw-posix: Implement .bdrv_lockf
  gluster: Implement .bdrv_lockf
  qemu-io: Add "-L" option for BDRV_O_NO_LOCK
  qemu-img: Add "-L" option to sub commands
  qemu-img: Update documentation of "-L" option
  qemu-nbd: Add "--no-lock/-L" option
  block: Don't lock drive-backup target image in none mode
  mirror: Disable image locking on target backing chain
  qemu-iotests: 140: Disable image lock for qemu-io access
  qemu-iotests: 046: Move version detection out from verify_io
  qemu-iotests: Wait for QEMU processes before checking image in 091
  qemu-iotests: 030: Disable image lock when checking test image
  iotests: 087: Disable image lock in cases where file is shared
  iotests: Disable image locking in 085
  tests: Use null-co:// instead of /dev/null
  block: Turn on image locking by default
  qemu-iotests: Add test case 153 for image locking

 block.c                    |  70 +++++++-
 block/gluster.c            |  31 ++++
 block/raw-posix.c          |  32 +++-
 blockdev.c                 |  34 ++++
 include/block/block.h      |   7 +-
 include/block/block_int.h  |  11 ++
 include/qemu/osdep.h       |   3 +
 qapi/block-core.json       |  20 ++-
 qemu-img-cmds.hx           |  44 ++---
 qemu-img.c                 |  90 ++++++++--
 qemu-img.texi              |   3 +
 qemu-io.c                  |  24 ++-
 qemu-nbd.c                 |   7 +-
 qemu-nbd.texi              |   2 +
 tests/drive_del-test.c     |   2 +-
 tests/nvme-test.c          |   2 +-
 tests/qemu-iotests/030     |   2 +-
 tests/qemu-iotests/046     |  22 +--
 tests/qemu-iotests/085     |   3 +-
 tests/qemu-iotests/087     |   6 +
 tests/qemu-iotests/091     |   3 +
 tests/qemu-iotests/091.out |   1 +
 tests/qemu-iotests/140     |   2 +-
 tests/qemu-iotests/153     | 197 +++++++++++++++++++++
 tests/qemu-iotests/153.out | 426 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/group   |   1 +
 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   |   4 +-
 util/osdep.c               |  37 ++++
 31 files changed, 1014 insertions(+), 78 deletions(-)
 create mode 100755 tests/qemu-iotests/153
 create mode 100644 tests/qemu-iotests/153.out

-- 
2.8.2

[Qemu-devel] [PATCH v5 01/27] block: Add flag bits for image locking

[Fam Zheng]

Later the block layer will automatically lock the images to avoid unexpected
concurrent accesses to the same image, which will easily corrupt the metadata
or user data, unless in some very special cases, like migration.

The exceptional cases like shared storage migration and testing should
set BDRV_O_SHARED_LOCK or BDRV_O_NO_LOCK to advise an appropriate
locking mode.

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 b210832..14f7300 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -94,6 +94,8 @@ typedef struct HDGeometry {
                                       select an appropriate protocol driver,
                                       ignoring the format layer */
 #define BDRV_O_NO_IO       0x10000 /* don't initialize for I/O */
+#define BDRV_O_NO_LOCK     0x20000 /* don't lock image file */
+#define BDRV_O_SHARED_LOCK 0x40000 /* lock the image file in shared mode */
 
 #define BDRV_O_CACHE_MASK  (BDRV_O_NOCACHE | BDRV_O_NO_FLUSH)
 
-- 
2.8.2

Re: [Qemu-devel] [PATCH v5 01/27] block: Add flag bits for image locking

[Max Reitz]

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

On 17.05.2016 09:35, Fam Zheng wrote:
> Later the block layer will automatically lock the images to avoid unexpected
> concurrent accesses to the same image, which will easily corrupt the metadata
> or user data, unless in some very special cases, like migration.
> 
> The exceptional cases like shared storage migration and testing should
> set BDRV_O_SHARED_LOCK or BDRV_O_NO_LOCK to advise an appropriate
> locking mode.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  include/block/block.h | 2 ++
>  1 file changed, 2 insertions(+)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

[Qemu-devel] [PATCH v5 02/27] qapi: Add lock-mode in blockdev-add options

[Fam Zheng]

To allow overriding the default locking behavior when opening the image.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 qapi/block-core.json | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 98a20d2..3c54f64 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2032,6 +2032,21 @@
             '*read-pattern': 'QuorumReadPattern' } }
 
 ##
+# @BlockdevLockMode
+#
+# Describes the operation mode for the automatic conversion of plain
+# zero writes by the OS to driver specific optimized zero write commands.
+#
+# @off:       Disabled
+# @shared:    Use shared lock for both RO and RW images.
+# @exclusive: Use exclusive lock for RW images, and shared lock for RO images.
+#
+# Since: 2.7
+##
+{ 'enum': 'BlockdevLockMode',
+  'data': [ 'off', 'shared', 'exclusive' ] }
+
+##
 # @BlockdevOptions
 #
 # Options for creating a block device.  Many options are available for all
@@ -2065,6 +2080,8 @@
 # @detect-zeroes: #optional detect and optimize zero writes (Since 2.1)
 #                 (default: off)
 #
+# @lock-mode: #optional how to lock the image. (default: exclusive) (Since 2.7)
+#
 # Remaining options are determined by the block driver.
 #
 # Since: 1.7
@@ -2082,7 +2099,8 @@
             '*stats-account-invalid': 'bool',
             '*stats-account-failed': 'bool',
             '*stats-intervals': ['int'],
-            '*detect-zeroes': 'BlockdevDetectZeroesOptions' },
+            '*detect-zeroes': 'BlockdevDetectZeroesOptions',
+            '*lock-mode': 'BlockdevLockMode' },
   'discriminator': 'driver',
   'data': {
       'archipelago':'BlockdevOptionsArchipelago',
-- 
2.8.2

Re: [Qemu-devel] [PATCH v5 02/27] qapi: Add lock-mode in blockdev-add options

[Max Reitz]

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

On 17.05.2016 09:35, Fam Zheng wrote:
> To allow overriding the default locking behavior when opening the image.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  qapi/block-core.json | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 98a20d2..3c54f64 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2032,6 +2032,21 @@
>              '*read-pattern': 'QuorumReadPattern' } }
>  
>  ##
> +# @BlockdevLockMode
> +#
> +# Describes the operation mode for the automatic conversion of plain
> +# zero writes by the OS to driver specific optimized zero write commands.

I don't think so. ;-)

Max

> +#
> +# @off:       Disabled
> +# @shared:    Use shared lock for both RO and RW images.
> +# @exclusive: Use exclusive lock for RW images, and shared lock for RO images.
> +#
> +# Since: 2.7
> +##
> +{ 'enum': 'BlockdevLockMode',
> +  'data': [ 'off', 'shared', 'exclusive' ] }
> +
> +##
>  # @BlockdevOptions
>  #
>  # Options for creating a block device.  Many options are available for all
> @@ -2065,6 +2080,8 @@
>  # @detect-zeroes: #optional detect and optimize zero writes (Since 2.1)
>  #                 (default: off)
>  #
> +# @lock-mode: #optional how to lock the image. (default: exclusive) (Since 2.7)
> +#
>  # Remaining options are determined by the block driver.
>  #
>  # Since: 1.7
> @@ -2082,7 +2099,8 @@
>              '*stats-account-invalid': 'bool',
>              '*stats-account-failed': 'bool',
>              '*stats-intervals': ['int'],
> -            '*detect-zeroes': 'BlockdevDetectZeroesOptions' },
> +            '*detect-zeroes': 'BlockdevDetectZeroesOptions',
> +            '*lock-mode': 'BlockdevLockMode' },
>    'discriminator': 'driver',
>    'data': {
>        'archipelago':'BlockdevOptionsArchipelago',
> 



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

[Qemu-devel] [PATCH v5 03/27] blockdev: Add and parse "lock-mode" option for image locking

[Fam Zheng]

Respect the locking mode from CLI or QMP, and set the open flags
accordingly.

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

diff --git a/blockdev.c b/blockdev.c
index 1892b8e..0784c4a 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -356,6 +356,7 @@ static void extract_common_blockdev_options(QemuOpts *opts, int *bdrv_flags,
     const char *discard;
     Error *local_error = NULL;
     const char *aio;
+    const char *lock_mode;
 
     if (bdrv_flags) {
         if (!qemu_opt_get_bool(opts, "read-only", false)) {
@@ -382,6 +383,18 @@ static void extract_common_blockdev_options(QemuOpts *opts, int *bdrv_flags,
                return;
             }
         }
+
+        lock_mode = qemu_opt_get(opts, "lock-mode") ? : "off";
+        if (!strcmp(lock_mode, "exclusive")) {
+            /* Default */
+        } else if (!strcmp(lock_mode, "shared")) {
+            *bdrv_flags |= BDRV_O_SHARED_LOCK;
+        } else if (!strcmp(lock_mode, "off")) {
+            *bdrv_flags |= BDRV_O_NO_LOCK;
+        } else {
+           error_setg(errp, "invalid lock mode");
+           return;
+        }
     }
 
     /* disk I/O throttling */
@@ -4304,6 +4317,11 @@ QemuOptsList qemu_common_drive_opts = {
             .type = QEMU_OPT_BOOL,
             .help = "whether to account for failed I/O operations "
                     "in the statistics",
+        },{
+            .name = "lock-mode",
+            .type = QEMU_OPT_STRING,
+            .help = "how to lock the image (exclusive, shared, off. "
+                    "default: exclusive)",
         },
         { /* end of list */ }
     },
@@ -4333,6 +4351,11 @@ static QemuOptsList qemu_root_bds_opts = {
             .name = "detect-zeroes",
             .type = QEMU_OPT_STRING,
             .help = "try to optimize zero writes (off, on, unmap)",
+        },{
+            .name = "lock-mode",
+            .type = QEMU_OPT_STRING,
+            .help = "how to lock the image (exclusive, shared, off. "
+                    "default: exclusive)",
         },
         { /* end of list */ }
     },
-- 
2.8.2

Re: [Qemu-devel] [PATCH v5 03/27] blockdev: Add and parse "lock-mode" option for image locking

[Max Reitz]

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

On 17.05.2016 09:35, Fam Zheng wrote:
> Respect the locking mode from CLI or QMP, and set the open flags
> accordingly.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  blockdev.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

[Qemu-devel] [PATCH v5 04/27] block: Introduce image file locking

[Fam Zheng]

Block drivers can implement this new operation .bdrv_lockf to actually lock the
image in the protocol specific way.

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

diff --git a/block.c b/block.c
index 18a497f..153f9d2 100644
--- a/block.c
+++ b/block.c
@@ -844,6 +844,41 @@ out:
     g_free(gen_node_name);
 }
 
+static int bdrv_lock_unlock_image_do(BlockDriverState *bs, bool lock_image)
+{
+    int cmd = BDRV_LOCKF_UNLOCK;
+    int ret;
+
+    if (bs->image_locked == lock_image) {
+        return 0;
+    } else if (!bs->drv) {
+        return -ENOMEDIUM;
+    } else if (!bs->drv->bdrv_lockf) {
+        return 0;
+    }
+    if (lock_image) {
+        cmd = (bs->open_flags & BDRV_O_RDWR) &&
+              !(bs->open_flags & BDRV_O_SHARED_LOCK) ? BDRV_LOCKF_EXCLUSIVE :
+                                                       BDRV_LOCKF_SHARED;
+    }
+    ret = bs->drv->bdrv_lockf(bs, cmd);
+    if (ret == -ENOTSUP) {
+        /* Handle it the same way as !bs->drv->bdrv_lockf */
+        ret = 0;
+    }
+    return ret;
+}
+
+static int bdrv_lock_image(BlockDriverState *bs)
+{
+    return bdrv_lock_unlock_image_do(bs, true);
+}
+
+static int bdrv_unlock_image(BlockDriverState *bs)
+{
+    return bdrv_lock_unlock_image_do(bs, false);
+}
+
 static QemuOptsList bdrv_runtime_opts = {
     .name = "bdrv_common",
     .head = QTAILQ_HEAD_INITIALIZER(bdrv_runtime_opts.head),
@@ -993,6 +1028,14 @@ static int bdrv_open_common(BlockDriverState *bs, BdrvChild *file,
         goto free_and_fail;
     }
 
+    if (!(open_flags & (BDRV_O_NO_LOCK | BDRV_O_INACTIVE))) {
+        ret = bdrv_lock_image(bs);
+        if (ret) {
+            error_setg(errp, "Failed to lock image");
+            goto free_and_fail;
+        }
+    }
+
     ret = refresh_total_sectors(bs, bs->total_sectors);
     if (ret < 0) {
         error_setg_errno(errp, -ret, "Could not refresh total sector count");
@@ -2142,6 +2185,7 @@ static void bdrv_close(BlockDriverState *bs)
     if (bs->drv) {
         BdrvChild *child, *next;
 
+        bdrv_unlock_image(bs);
         bs->drv->bdrv_close(bs);
         bs->drv = NULL;
 
@@ -3235,6 +3279,9 @@ void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp)
         error_setg_errno(errp, -ret, "Could not refresh total sector count");
         return;
     }
+    if (!(bs->open_flags & BDRV_O_NO_LOCK)) {
+        bdrv_lock_image(bs);
+    }
 }
 
 void bdrv_invalidate_cache_all(Error **errp)
@@ -3276,6 +3323,7 @@ static int bdrv_inactivate_recurse(BlockDriverState *bs,
     }
 
     if (setting_flag) {
+        ret = bdrv_unlock_image(bs);
         bs->open_flags |= BDRV_O_INACTIVE;
     }
     return 0;
diff --git a/include/block/block_int.h b/include/block/block_int.h
index a029c20..2b82d49 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -85,6 +85,12 @@ typedef struct BdrvTrackedRequest {
     struct BdrvTrackedRequest *waiting_for;
 } BdrvTrackedRequest;
 
+typedef enum {
+    BDRV_LOCKF_EXCLUSIVE,
+    BDRV_LOCKF_SHARED,
+    BDRV_LOCKF_UNLOCK,
+} BdrvLockfCmd;
+
 struct BlockDriver {
     const char *format_name;
     int instance_size;
@@ -318,6 +324,10 @@ struct BlockDriver {
                            Error **errp);
     void (*bdrv_del_child)(BlockDriverState *parent, BdrvChild *child,
                            Error **errp);
+    /**
+     * Lock/unlock the image.
+     */
+    int (*bdrv_lockf)(BlockDriverState *bs, BdrvLockfCmd cmd);
 
     QLIST_ENTRY(BlockDriver) list;
 };
@@ -496,6 +506,7 @@ struct BlockDriverState {
     unsigned io_plug_disabled;
 
     int quiesce_counter;
+    bool image_locked;
 };
 
 struct BlockBackendRootState {
-- 
2.8.2

Re: [Qemu-devel] [PATCH v5 04/27] block: Introduce image file locking

[Max Reitz]

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

On 17.05.2016 09:35, Fam Zheng wrote:
> Block drivers can implement this new operation .bdrv_lockf to actually lock the
> image in the protocol specific way.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block.c                   | 48 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/block/block_int.h | 11 +++++++++++
>  2 files changed, 59 insertions(+)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

[Qemu-devel] [PATCH v5 05/27] block: Add bdrv_image_locked

[Fam Zheng]

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

diff --git a/block.c b/block.c
index 153f9d2..1b42303 100644
--- a/block.c
+++ b/block.c
@@ -879,6 +879,11 @@ static int bdrv_unlock_image(BlockDriverState *bs)
     return bdrv_lock_unlock_image_do(bs, false);
 }
 
+bool bdrv_image_locked(BlockDriverState *bs)
+{
+    return bs->image_locked;
+}
+
 static QemuOptsList bdrv_runtime_opts = {
     .name = "bdrv_common",
     .head = QTAILQ_HEAD_INITIALIZER(bdrv_runtime_opts.head),
diff --git a/include/block/block.h b/include/block/block.h
index 14f7300..7a7dfb5 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -283,7 +283,7 @@ int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top,
 BlockDriverState *bdrv_find_overlay(BlockDriverState *active,
                                     BlockDriverState *bs);
 BlockDriverState *bdrv_find_base(BlockDriverState *bs);
-
+bool bdrv_image_locked(BlockDriverState *bs);
 
 typedef struct BdrvCheckResult {
     int corruptions;
-- 
2.8.2

Re: [Qemu-devel] [PATCH v5 05/27] block: Add bdrv_image_locked

[Max Reitz]

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

On 17.05.2016 09:35, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block.c               | 5 +++++
>  include/block/block.h | 2 +-
>  2 files changed, 6 insertions(+), 1 deletion(-)

Maybe this should recurse down bs->file (or just all children) if the
BDS is not locked?

If you don't need such behavior:

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

[Qemu-devel] [PATCH v5 06/27] block: Make bdrv_reopen_{commit, abort} private functions

[Fam Zheng]

As parts of the transactional reopen, they are not necessary outside
block.c. Make them static.

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

diff --git a/block.c b/block.c
index 1b42303..ad3663c 100644
--- a/block.c
+++ b/block.c
@@ -1943,6 +1943,8 @@ BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
  * to all devices.
  *
  */
+static void bdrv_reopen_commit(BDRVReopenState *reopen_state);
+static void bdrv_reopen_abort(BDRVReopenState *reopen_state);
 int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp)
 {
     int ret = -1;
@@ -2122,7 +2124,7 @@ error:
  * makes them final by swapping the staging BlockDriverState contents into
  * the active BlockDriverState contents.
  */
-void bdrv_reopen_commit(BDRVReopenState *reopen_state)
+static void bdrv_reopen_commit(BDRVReopenState *reopen_state)
 {
     BlockDriver *drv;
 
@@ -2149,7 +2151,7 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
  * Abort the reopen, and delete and free the staged changes in
  * reopen_state
  */
-void bdrv_reopen_abort(BDRVReopenState *reopen_state)
+static void bdrv_reopen_abort(BDRVReopenState *reopen_state)
 {
     BlockDriver *drv;
 
diff --git a/include/block/block.h b/include/block/block.h
index 7a7dfb5..7740d3f 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -228,8 +228,6 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp);
 int bdrv_reopen(BlockDriverState *bs, int bdrv_flags, Error **errp);
 int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
                         BlockReopenQueue *queue, Error **errp);
-void bdrv_reopen_commit(BDRVReopenState *reopen_state);
-void bdrv_reopen_abort(BDRVReopenState *reopen_state);
 int bdrv_read(BlockDriverState *bs, int64_t sector_num,
               uint8_t *buf, int nb_sectors);
 int bdrv_write(BlockDriverState *bs, int64_t sector_num,
-- 
2.8.2

Re: [Qemu-devel] [PATCH v5 06/27] block: Make bdrv_reopen_{commit, abort} private functions

[Max Reitz]

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

On 17.05.2016 09:35, Fam Zheng wrote:
> As parts of the transactional reopen, they are not necessary outside
> block.c. Make them static.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block.c               | 6 ++++--
>  include/block/block.h | 2 --
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 1b42303..ad3663c 100644
> --- a/block.c
> +++ b/block.c
> @@ -1943,6 +1943,8 @@ BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
>   * to all devices.
>   *
>   */
> +static void bdrv_reopen_commit(BDRVReopenState *reopen_state);
> +static void bdrv_reopen_abort(BDRVReopenState *reopen_state);

I'd rather put these declarations above the comment describing
bdrv_reopen_multiple(); or just at the top of the file, we have two such
declarations there already.

Max

>  int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp)
>  {
>      int ret = -1;
> @@ -2122,7 +2124,7 @@ error:
>   * makes them final by swapping the staging BlockDriverState contents into
>   * the active BlockDriverState contents.
>   */
> -void bdrv_reopen_commit(BDRVReopenState *reopen_state)
> +static void bdrv_reopen_commit(BDRVReopenState *reopen_state)
>  {
>      BlockDriver *drv;
>  
> @@ -2149,7 +2151,7 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
>   * Abort the reopen, and delete and free the staged changes in
>   * reopen_state
>   */
> -void bdrv_reopen_abort(BDRVReopenState *reopen_state)
> +static void bdrv_reopen_abort(BDRVReopenState *reopen_state)
>  {
>      BlockDriver *drv;
>  
> diff --git a/include/block/block.h b/include/block/block.h
> index 7a7dfb5..7740d3f 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -228,8 +228,6 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp);
>  int bdrv_reopen(BlockDriverState *bs, int bdrv_flags, Error **errp);
>  int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
>                          BlockReopenQueue *queue, Error **errp);
> -void bdrv_reopen_commit(BDRVReopenState *reopen_state);
> -void bdrv_reopen_abort(BDRVReopenState *reopen_state);
>  int bdrv_read(BlockDriverState *bs, int64_t sector_num,
>                uint8_t *buf, int nb_sectors);
>  int bdrv_write(BlockDriverState *bs, int64_t sector_num,
> 



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

Re: [Qemu-devel] [PATCH v5 06/27] block: Make bdrv_reopen_{commit, abort} private functions

[Fam Zheng]

On Tue, 05/24 18:09, Max Reitz wrote:
> On 17.05.2016 09:35, Fam Zheng wrote:
> > As parts of the transactional reopen, they are not necessary outside
> > block.c. Make them static.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  block.c               | 6 ++++--
> >  include/block/block.h | 2 --
> >  2 files changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/block.c b/block.c
> > index 1b42303..ad3663c 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -1943,6 +1943,8 @@ BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
> >   * to all devices.
> >   *
> >   */
> > +static void bdrv_reopen_commit(BDRVReopenState *reopen_state);
> > +static void bdrv_reopen_abort(BDRVReopenState *reopen_state);
> 
> I'd rather put these declarations above the comment describing
> bdrv_reopen_multiple(); or just at the top of the file, we have two such
> declarations there already.

Right, moving them to above the comment.

Fam

[Qemu-devel] [PATCH v5 07/27] block: Handle image locking during reopen

[Fam Zheng]

Stash the locking state into BDRVReopenState. If it was locked, unlock
in prepare, and lock it again when commit or abort.

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

diff --git a/block.c b/block.c
index ad3663c..2be42bb 100644
--- a/block.c
+++ b/block.c
@@ -2112,6 +2112,11 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
         } while ((entry = qdict_next(reopen_state->options, entry)));
     }
 
+    reopen_state->was_locked = reopen_state->bs->image_locked;
+    if (reopen_state->was_locked) {
+        bdrv_unlock_image(reopen_state->bs);
+    }
+
     ret = 0;
 
 error:
@@ -2136,6 +2141,9 @@ static void bdrv_reopen_commit(BDRVReopenState *reopen_state)
     if (drv->bdrv_reopen_commit) {
         drv->bdrv_reopen_commit(reopen_state);
     }
+    if (reopen_state->was_locked) {
+        bdrv_lock_image(reopen_state->bs);
+    }
 
     /* set BDS specific flags now */
     QDECREF(reopen_state->bs->explicit_options);
@@ -2162,6 +2170,9 @@ static void bdrv_reopen_abort(BDRVReopenState *reopen_state)
     if (drv->bdrv_reopen_abort) {
         drv->bdrv_reopen_abort(reopen_state);
     }
+    if (reopen_state->was_locked) {
+        bdrv_lock_image(reopen_state->bs);
+    }
 
     QDECREF(reopen_state->explicit_options);
 }
diff --git a/include/block/block.h b/include/block/block.h
index 7740d3f..28b8ae9 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -158,6 +158,7 @@ typedef struct BDRVReopenState {
     QDict *options;
     QDict *explicit_options;
     void *opaque;
+    bool was_locked;
 } BDRVReopenState;
 
 /*
-- 
2.8.2

Re: [Qemu-devel] [PATCH v5 07/27] block: Handle image locking during reopen

[Max Reitz]

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

On 17.05.2016 09:35, Fam Zheng wrote:
> Stash the locking state into BDRVReopenState. If it was locked, unlock
> in prepare, and lock it again when commit or abort.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block.c               | 11 +++++++++++
>  include/block/block.h |  1 +
>  2 files changed, 12 insertions(+)
> 
> diff --git a/block.c b/block.c
> index ad3663c..2be42bb 100644
> --- a/block.c
> +++ b/block.c
> @@ -2112,6 +2112,11 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
>          } while ((entry = qdict_next(reopen_state->options, entry)));
>      }
>  
> +    reopen_state->was_locked = reopen_state->bs->image_locked;
> +    if (reopen_state->was_locked) {
> +        bdrv_unlock_image(reopen_state->bs);
> +    }
> +
>      ret = 0;
>  
>  error:
> @@ -2136,6 +2141,9 @@ static void bdrv_reopen_commit(BDRVReopenState *reopen_state)
>      if (drv->bdrv_reopen_commit) {
>          drv->bdrv_reopen_commit(reopen_state);
>      }
> +    if (reopen_state->was_locked) {
> +        bdrv_lock_image(reopen_state->bs);
> +    }
>  
>      /* set BDS specific flags now */
>      QDECREF(reopen_state->bs->explicit_options);
> @@ -2162,6 +2170,9 @@ static void bdrv_reopen_abort(BDRVReopenState *reopen_state)
>      if (drv->bdrv_reopen_abort) {
>          drv->bdrv_reopen_abort(reopen_state);
>      }
> +    if (reopen_state->was_locked) {
> +        bdrv_lock_image(reopen_state->bs);

I'm not sure I'm quite happy with this, because this may fail;
therefore, it may happen that the operation done in _prepare() cannot be
undone.

Of course, the same applies to _commit(): bdrv_lock_image() can just
fail. It's probably even worse there. I don't see a good reason why just
relocking the image in _abort() should fail; but it's very much
conceivable that locking the reopened image in _commit() fails.

I think the correct way would be to rely on the drivers which implement
locking to handle reopening correctly, that is, try lock the reopened
image in _prepare() and unlock the old one before discarding it in
_commit().

However, I'm not sure myself whether it's worth the effort. It's very
simple to do it like you did here -- but then again, it doesn't seem
quite correct.

Also: Should bdrv_reopen_prepare() check that the locking flags are not
changed?

Max

> +    }
>  
>      QDECREF(reopen_state->explicit_options);
>  }
> diff --git a/include/block/block.h b/include/block/block.h
> index 7740d3f..28b8ae9 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -158,6 +158,7 @@ typedef struct BDRVReopenState {
>      QDict *options;
>      QDict *explicit_options;
>      void *opaque;
> +    bool was_locked;
>  } BDRVReopenState;
>  
>  /*
> 



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

Re: [Qemu-devel] [PATCH v5 07/27] block: Handle image locking during reopen

[Fam Zheng]

On Tue, 05/24 18:28, Max Reitz wrote:
> On 17.05.2016 09:35, Fam Zheng wrote:
> > Stash the locking state into BDRVReopenState. If it was locked, unlock
> > in prepare, and lock it again when commit or abort.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  block.c               | 11 +++++++++++
> >  include/block/block.h |  1 +
> >  2 files changed, 12 insertions(+)
> > 
> > diff --git a/block.c b/block.c
> > index ad3663c..2be42bb 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -2112,6 +2112,11 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
> >          } while ((entry = qdict_next(reopen_state->options, entry)));
> >      }
> >  
> > +    reopen_state->was_locked = reopen_state->bs->image_locked;
> > +    if (reopen_state->was_locked) {
> > +        bdrv_unlock_image(reopen_state->bs);
> > +    }
> > +
> >      ret = 0;
> >  
> >  error:
> > @@ -2136,6 +2141,9 @@ static void bdrv_reopen_commit(BDRVReopenState *reopen_state)
> >      if (drv->bdrv_reopen_commit) {
> >          drv->bdrv_reopen_commit(reopen_state);
> >      }
> > +    if (reopen_state->was_locked) {
> > +        bdrv_lock_image(reopen_state->bs);
> > +    }
> >  
> >      /* set BDS specific flags now */
> >      QDECREF(reopen_state->bs->explicit_options);
> > @@ -2162,6 +2170,9 @@ static void bdrv_reopen_abort(BDRVReopenState *reopen_state)
> >      if (drv->bdrv_reopen_abort) {
> >          drv->bdrv_reopen_abort(reopen_state);
> >      }
> > +    if (reopen_state->was_locked) {
> > +        bdrv_lock_image(reopen_state->bs);
> 
> I'm not sure I'm quite happy with this, because this may fail;
> therefore, it may happen that the operation done in _prepare() cannot be
> undone.
> 
> Of course, the same applies to _commit(): bdrv_lock_image() can just
> fail. It's probably even worse there. I don't see a good reason why just
> relocking the image in _abort() should fail; but it's very much
> conceivable that locking the reopened image in _commit() fails.
> 
> I think the correct way would be to rely on the drivers which implement
> locking to handle reopening correctly, that is, try lock the reopened
> image in _prepare() and unlock the old one before discarding it in
> _commit().
> 
> However, I'm not sure myself whether it's worth the effort. It's very
> simple to do it like you did here -- but then again, it doesn't seem
> quite correct.

You are right, this is driver specific. raw-posix should "update" the lock.
Will update this patch.

> 
> Also: Should bdrv_reopen_prepare() check that the locking flags are not
> changed?

Read only flag also matters in fcntl locks, so practically we almost always
need some change on the lock.

Fam

Re: [Qemu-devel] [PATCH v5 07/27] block: Handle image locking during reopen

[Max Reitz]

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

On 27.05.2016 09:48, Fam Zheng wrote:
> On Tue, 05/24 18:28, Max Reitz wrote:

[...]

>> Also: Should bdrv_reopen_prepare() check that the locking flags are not
>> changed?
> 
> Read only flag also matters in fcntl locks, so practically we almost always
> need some change on the lock.

Hm, but as far as I can see, you don't handle changed locking flags
here; the reopened image is just locked if the old one had been locked
before. Since the handling of BDRV_O_SHARED_LOCK is done in
bdrv_lock_unlock_image_do(), that means that BDRV_O_NO_LOCK is ignored
if changed, isn't it?

Max


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

Re: [Qemu-devel] [PATCH v5 07/27] block: Handle image locking during reopen

[Fam Zheng]

On Fri, 05/27 11:57, Max Reitz wrote:
> On 27.05.2016 09:48, Fam Zheng wrote:
> > On Tue, 05/24 18:28, Max Reitz wrote:
> 
> [...]
> 
> >> Also: Should bdrv_reopen_prepare() check that the locking flags are not
> >> changed?
> > 
> > Read only flag also matters in fcntl locks, so practically we almost always
> > need some change on the lock.
> 
> Hm, but as far as I can see, you don't handle changed locking flags
> here; the reopened image is just locked if the old one had been locked
> before. Since the handling of BDRV_O_SHARED_LOCK is done in
> bdrv_lock_unlock_image_do(), that means that BDRV_O_NO_LOCK is ignored
> if changed, isn't it?

Yes you are rigth. I'm reworking this patch and it should fix this problem too.

Fam

[Qemu-devel] [PATCH v5 08/27] osdep: Add qemu_lock_fd and qemu_unlock_fd

[Fam Zheng]

They are wrappers of POSIX fcntl "file private locking".

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 include/qemu/osdep.h |  2 ++
 util/osdep.c         | 31 +++++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 1e3221c..81913a7 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -265,6 +265,8 @@ int qemu_madvise(void *addr, size_t len, int advice);
 
 int qemu_open(const char *name, int flags, ...);
 int qemu_close(int fd);
+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);
 
 #if defined(__HAIKU__) && defined(__i386__)
 #define FMT_pid "%ld"
diff --git a/util/osdep.c b/util/osdep.c
index d56d071..9e5d7fa 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -236,6 +236,37 @@ int qemu_close(int fd)
     return close(fd);
 }
 
+static int qemu_lock_fcntl(int fd, int64_t start, int64_t len, int fl_type)
+{
+#ifdef F_OFD_SETLK
+    struct flock fl = (struct flock) {
+        .l_whence  = SEEK_SET,
+        /* Locking byte 1 avoids interfereing with virtlockd. */
+        .l_start = start,
+        .l_len = len,
+        .l_type = fl_type,
+    };
+    if (fcntl(fd, F_OFD_SETLK, &fl) == -1) {
+        return -errno;
+    } else {
+        return 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);
+}
+
 /*
  * A variant of write(2) which handles partial write.
  *
-- 
2.8.2

Re: [Qemu-devel] [PATCH v5 08/27] osdep: Add qemu_lock_fd and qemu_unlock_fd

[Max Reitz]

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

On 17.05.2016 09:35, Fam Zheng wrote:
> They are wrappers of POSIX fcntl "file private locking".
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  include/qemu/osdep.h |  2 ++
>  util/osdep.c         | 31 +++++++++++++++++++++++++++++++
>  2 files changed, 33 insertions(+)
> 
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index 1e3221c..81913a7 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -265,6 +265,8 @@ int qemu_madvise(void *addr, size_t len, int advice);
>  
>  int qemu_open(const char *name, int flags, ...);
>  int qemu_close(int fd);
> +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);
>  
>  #if defined(__HAIKU__) && defined(__i386__)
>  #define FMT_pid "%ld"
> diff --git a/util/osdep.c b/util/osdep.c
> index d56d071..9e5d7fa 100644
> --- a/util/osdep.c
> +++ b/util/osdep.c
> @@ -236,6 +236,37 @@ int qemu_close(int fd)
>      return close(fd);
>  }
>  
> +static int qemu_lock_fcntl(int fd, int64_t start, int64_t len, int fl_type)
> +{
> +#ifdef F_OFD_SETLK
> +    struct flock fl = (struct flock) {

The (struct flock) can be omitted here (since this is an
initialization), but that's up to you.

> +        .l_whence  = SEEK_SET,

One space too many?

> +        /* Locking byte 1 avoids interfereing with virtlockd. */

*interfering or *interference

But apart from that: This comment seems completely misplaced here. I
guess it's supposed to be in raw-posix.c instead (or just wherever
you're calling qemu_{un,}lock_fd()).

> +        .l_start = start,
> +        .l_len = len,
> +        .l_type = fl_type,
> +    };
> +    if (fcntl(fd, F_OFD_SETLK, &fl) == -1) {
> +        return -errno;
> +    } else {
> +        return 0;
> +    }
> +#else
> +    return -ENOTSUP;
> +#endif
> +}
> +
> +int qemu_lock_fd(int fd, int64_t start, int64_t len, bool exclusive)
> +{
> +

Superfluous empty line.

Max

> +    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);
> +}
> +
>  /*
>   * A variant of write(2) which handles partial write.
>   *
> 



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

[Qemu-devel] [PATCH v5 09/27] osdep: Introduce qemu_dup

[Fam Zheng]

This takes care both the CLOEXEC flag and fd-path mapping for image
locking.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 include/qemu/osdep.h | 1 +
 util/osdep.c         | 6 ++++++
 2 files changed, 7 insertions(+)

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 81913a7..0e51279 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -266,6 +266,7 @@ int qemu_madvise(void *addr, size_t len, int advice);
 int qemu_open(const char *name, int flags, ...);
 int qemu_close(int fd);
 int qemu_lock_fd(int fd, int64_t start, int64_t len, bool exclusive);
+int qemu_dup(int fd);
 int qemu_unlock_fd(int fd, int64_t start, int64_t len);
 
 #if defined(__HAIKU__) && defined(__i386__)
diff --git a/util/osdep.c b/util/osdep.c
index 9e5d7fa..966bc32 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -267,6 +267,12 @@ int qemu_unlock_fd(int fd, int64_t start, int64_t len)
     return qemu_lock_fcntl(fd, start, len, F_UNLCK);
 }
 
+int qemu_dup(int fd)
+{
+    return qemu_dup_flags(fd, 0);
+}
+
+
 /*
  * A variant of write(2) which handles partial write.
  *
-- 
2.8.2

Re: [Qemu-devel] [PATCH v5 09/27] osdep: Introduce qemu_dup

[Max Reitz]

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

On 17.05.2016 09:35, Fam Zheng wrote:
> This takes care

+of

>                 both the CLOEXEC flag and fd-path mapping for image
> locking.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  include/qemu/osdep.h | 1 +
>  util/osdep.c         | 6 ++++++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index 81913a7..0e51279 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -266,6 +266,7 @@ int qemu_madvise(void *addr, size_t len, int advice);
>  int qemu_open(const char *name, int flags, ...);
>  int qemu_close(int fd);
>  int qemu_lock_fd(int fd, int64_t start, int64_t len, bool exclusive);
> +int qemu_dup(int fd);
>  int qemu_unlock_fd(int fd, int64_t start, int64_t len);
>  
>  #if defined(__HAIKU__) && defined(__i386__)
> diff --git a/util/osdep.c b/util/osdep.c
> index 9e5d7fa..966bc32 100644
> --- a/util/osdep.c
> +++ b/util/osdep.c
> @@ -267,6 +267,12 @@ int qemu_unlock_fd(int fd, int64_t start, int64_t len)
>      return qemu_lock_fcntl(fd, start, len, F_UNLCK);
>  }
>  
> +int qemu_dup(int fd)
> +{
> +    return qemu_dup_flags(fd, 0);
> +}

I don't think we have this function on Windows. Maybe you don't need
qemu_dup() on Windows, then it would make sense to wrap it in an
appropriate #ifndef.

Also, I'm not sure how useful it is to return -1 on error with errno
being set. Since this is a public function, maybe it makes more sense to
return -errno then.

Max

> +
> +
>  /*
>   * A variant of write(2) which handles partial write.
>   *
> 



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

[Qemu-devel] [PATCH v5 10/27] raw-posix: Use qemu_dup

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/raw-posix.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index a4f5a1b..bb8669f 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -645,15 +645,7 @@ static int raw_reopen_prepare(BDRVReopenState *state,
 
     if ((raw_s->open_flags & ~fcntl_flags) == (s->open_flags & ~fcntl_flags)) {
         /* dup the original fd */
-        /* TODO: use qemu fcntl wrapper */
-#ifdef F_DUPFD_CLOEXEC
-        raw_s->fd = fcntl(s->fd, F_DUPFD_CLOEXEC, 0);
-#else
-        raw_s->fd = dup(s->fd);
-        if (raw_s->fd != -1) {
-            qemu_set_cloexec(raw_s->fd);
-        }
-#endif
+        raw_s->fd = qemu_dup(s->fd);
         if (raw_s->fd >= 0) {
             ret = fcntl_setfl(raw_s->fd, raw_s->open_flags);
             if (ret) {
-- 
2.8.2

Re: [Qemu-devel] [PATCH v5 10/27] raw-posix: Use qemu_dup

[Max Reitz]

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

On 17.05.2016 09:35, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/raw-posix.c | 10 +---------
>  1 file changed, 1 insertion(+), 9 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

[Qemu-devel] [PATCH v5 11/27] raw-posix: Implement .bdrv_lockf

[Fam Zheng]

virtlockd in libvirt locks the first byte, we lock byte 1 to avoid
the intervene.

Both file and host device protocols are covered.

Suggested-by: "Daniel P. Berrange" <berrange@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/raw-posix.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index bb8669f..acd3be2 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -35,6 +35,7 @@
 #include "raw-aio.h"
 #include "qapi/util.h"
 #include "qapi/qmp/qstring.h"
+#include "glib.h"
 
 #if defined(__APPLE__) && (__MACH__)
 #include <paths.h>
@@ -397,6 +398,23 @@ static void raw_attach_aio_context(BlockDriverState *bs,
 #endif
 }
 
+static int raw_lockf(BlockDriverState *bs, BdrvLockfCmd cmd)
+{
+
+    BDRVRawState *s = bs->opaque;
+
+    switch (cmd) {
+    case BDRV_LOCKF_EXCLUSIVE:
+        return qemu_lock_fd(s->fd, 1, 1, true);
+    case BDRV_LOCKF_SHARED:
+        return qemu_lock_fd(s->fd, 1, 1, false);
+    case BDRV_LOCKF_UNLOCK:
+        return qemu_unlock_fd(s->fd, 1, 1);
+    default:
+        abort();
+    }
+}
+
 #ifdef CONFIG_LINUX_AIO
 static int raw_set_aio(LinuxAioState **aio_ctx, int *use_aio, int bdrv_flags)
 {
@@ -1942,6 +1960,8 @@ BlockDriver bdrv_file = {
     .bdrv_detach_aio_context = raw_detach_aio_context,
     .bdrv_attach_aio_context = raw_attach_aio_context,
 
+    .bdrv_lockf = raw_lockf,
+
     .create_opts = &raw_create_opts,
 };
 
@@ -2396,6 +2416,8 @@ static BlockDriver bdrv_host_device = {
 #ifdef __linux__
     .bdrv_aio_ioctl     = hdev_aio_ioctl,
 #endif
+
+    .bdrv_lockf = raw_lockf,
 };
 
 #if defined(__linux__) || defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
-- 
2.8.2

Re: [Qemu-devel] [PATCH v5 11/27] raw-posix: Implement .bdrv_lockf

[Max Reitz]

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

On 17.05.2016 09:35, Fam Zheng wrote:
> virtlockd in libvirt locks the first byte, we lock byte 1 to avoid
> the intervene.
> 
> Both file and host device protocols are covered.

Is there a reason you didn't cover host_cdrom other than it generally
being read-only and thus probably not really needing a lock?

> Suggested-by: "Daniel P. Berrange" <berrange@redhat.com>

I think the quotation marks are superfluous.

> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/raw-posix.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index bb8669f..acd3be2 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -35,6 +35,7 @@
>  #include "raw-aio.h"
>  #include "qapi/util.h"
>  #include "qapi/qmp/qstring.h"
> +#include "glib.h"

What for?

>  
>  #if defined(__APPLE__) && (__MACH__)
>  #include <paths.h>
> @@ -397,6 +398,23 @@ static void raw_attach_aio_context(BlockDriverState *bs,
>  #endif
>  }
>  
> +static int raw_lockf(BlockDriverState *bs, BdrvLockfCmd cmd)
> +{
> +
> +    BDRVRawState *s = bs->opaque;
> +
> +    switch (cmd) {
> +    case BDRV_LOCKF_EXCLUSIVE:
> +        return qemu_lock_fd(s->fd, 1, 1, true);
> +    case BDRV_LOCKF_SHARED:
> +        return qemu_lock_fd(s->fd, 1, 1, false);
> +    case BDRV_LOCKF_UNLOCK:
> +        return qemu_unlock_fd(s->fd, 1, 1);
> +    default:
> +        abort();
> +    }

I figure the comment from patch 8 about why byte 1 is locked should be
here somewhere.

Max

> +}
> +
>  #ifdef CONFIG_LINUX_AIO
>  static int raw_set_aio(LinuxAioState **aio_ctx, int *use_aio, int bdrv_flags)
>  {
> @@ -1942,6 +1960,8 @@ BlockDriver bdrv_file = {
>      .bdrv_detach_aio_context = raw_detach_aio_context,
>      .bdrv_attach_aio_context = raw_attach_aio_context,
>  
> +    .bdrv_lockf = raw_lockf,
> +
>      .create_opts = &raw_create_opts,
>  };
>  
> @@ -2396,6 +2416,8 @@ static BlockDriver bdrv_host_device = {
>  #ifdef __linux__
>      .bdrv_aio_ioctl     = hdev_aio_ioctl,
>  #endif
> +
> +    .bdrv_lockf = raw_lockf,
>  };
>  
>  #if defined(__linux__) || defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
> 


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

Re: [Qemu-devel] [PATCH v5 11/27] raw-posix: Implement .bdrv_lockf

[Fam Zheng]

On Tue, 05/24 19:09, Max Reitz wrote:
> On 17.05.2016 09:35, Fam Zheng wrote:
> > virtlockd in libvirt locks the first byte, we lock byte 1 to avoid
> > the intervene.
> > 
> > Both file and host device protocols are covered.
> 
> Is there a reason you didn't cover host_cdrom other than it generally
> being read-only and thus probably not really needing a lock?

That is the reason.


> 
> > Suggested-by: "Daniel P. Berrange" <berrange@redhat.com>
> 
> I think the quotation marks are superfluous.

OK, I can remove them.

> 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  block/raw-posix.c | 22 ++++++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> > 
> > diff --git a/block/raw-posix.c b/block/raw-posix.c
> > index bb8669f..acd3be2 100644
> > --- a/block/raw-posix.c
> > +++ b/block/raw-posix.c
> > @@ -35,6 +35,7 @@
> >  #include "raw-aio.h"
> >  #include "qapi/util.h"
> >  #include "qapi/qmp/qstring.h"
> > +#include "glib.h"
> 
> What for?

It's stale from a previous revision.

> 
> >  
> >  #if defined(__APPLE__) && (__MACH__)
> >  #include <paths.h>
> > @@ -397,6 +398,23 @@ static void raw_attach_aio_context(BlockDriverState *bs,
> >  #endif
> >  }
> >  
> > +static int raw_lockf(BlockDriverState *bs, BdrvLockfCmd cmd)
> > +{
> > +
> > +    BDRVRawState *s = bs->opaque;
> > +
> > +    switch (cmd) {
> > +    case BDRV_LOCKF_EXCLUSIVE:
> > +        return qemu_lock_fd(s->fd, 1, 1, true);
> > +    case BDRV_LOCKF_SHARED:
> > +        return qemu_lock_fd(s->fd, 1, 1, false);
> > +    case BDRV_LOCKF_UNLOCK:
> > +        return qemu_unlock_fd(s->fd, 1, 1);
> > +    default:
> > +        abort();
> > +    }
> 
> I figure the comment from patch 8 about why byte 1 is locked should be
> here somewhere.

Yes.

Thanks,
Fam

[Qemu-devel] [PATCH v5 12/27] gluster: Implement .bdrv_lockf

[Fam Zheng]

Reviewed-by: Niels de Vos <ndevos@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/gluster.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/block/gluster.c b/block/gluster.c
index a8aaacf..517b4c3 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -723,6 +723,33 @@ static int64_t qemu_gluster_allocated_file_size(BlockDriverState *bs)
     }
 }
 
+
+static int qemu_gluster_lockf(BlockDriverState *bs, BdrvLockfCmd cmd)
+{
+    BDRVGlusterState *s = bs->opaque;
+    int ret;
+    struct flock fl = (struct flock) {
+        .l_start = 0,
+        .l_whence  = SEEK_SET,
+        .l_len = 0,
+    };
+    switch (cmd) {
+    case BDRV_LOCKF_EXCLUSIVE:
+        fl.l_type = F_WRLCK;
+        break;
+    case BDRV_LOCKF_SHARED:
+        fl.l_type = F_RDLCK;
+        break;
+    case BDRV_LOCKF_UNLOCK:
+        fl.l_type = F_UNLCK;
+        break;
+    default:
+        abort();
+    }
+    ret = glfs_posix_lock(s->fd, F_SETLK, &fl);
+    return ret == -1 ? -errno : 0;
+}
+
 static int qemu_gluster_has_zero_init(BlockDriverState *bs)
 {
     /* GlusterFS volume could be backed by a block device */
@@ -764,6 +791,7 @@ static BlockDriver bdrv_gluster = {
     .bdrv_co_readv                = qemu_gluster_co_readv,
     .bdrv_co_writev               = qemu_gluster_co_writev,
     .bdrv_co_flush_to_disk        = qemu_gluster_co_flush_to_disk,
+    .bdrv_lockf                   = qemu_gluster_lockf,
     .bdrv_has_zero_init           = qemu_gluster_has_zero_init,
 #ifdef CONFIG_GLUSTERFS_DISCARD
     .bdrv_co_discard              = qemu_gluster_co_discard,
@@ -791,6 +819,7 @@ static BlockDriver bdrv_gluster_tcp = {
     .bdrv_co_readv                = qemu_gluster_co_readv,
     .bdrv_co_writev               = qemu_gluster_co_writev,
     .bdrv_co_flush_to_disk        = qemu_gluster_co_flush_to_disk,
+    .bdrv_lockf                   = qemu_gluster_lockf,
     .bdrv_has_zero_init           = qemu_gluster_has_zero_init,
 #ifdef CONFIG_GLUSTERFS_DISCARD
     .bdrv_co_discard              = qemu_gluster_co_discard,
@@ -818,6 +847,7 @@ static BlockDriver bdrv_gluster_unix = {
     .bdrv_co_readv                = qemu_gluster_co_readv,
     .bdrv_co_writev               = qemu_gluster_co_writev,
     .bdrv_co_flush_to_disk        = qemu_gluster_co_flush_to_disk,
+    .bdrv_lockf                   = qemu_gluster_lockf,
     .bdrv_has_zero_init           = qemu_gluster_has_zero_init,
 #ifdef CONFIG_GLUSTERFS_DISCARD
     .bdrv_co_discard              = qemu_gluster_co_discard,
@@ -845,6 +875,7 @@ static BlockDriver bdrv_gluster_rdma = {
     .bdrv_co_readv                = qemu_gluster_co_readv,
     .bdrv_co_writev               = qemu_gluster_co_writev,
     .bdrv_co_flush_to_disk        = qemu_gluster_co_flush_to_disk,
+    .bdrv_lockf                   = qemu_gluster_lockf,
     .bdrv_has_zero_init           = qemu_gluster_has_zero_init,
 #ifdef CONFIG_GLUSTERFS_DISCARD
     .bdrv_co_discard              = qemu_gluster_co_discard,
-- 
2.8.2

[Qemu-devel] [PATCH v5 13/27] qemu-io: Add "-L" option for BDRV_O_NO_LOCK

[Fam Zheng]

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

diff --git a/qemu-io.c b/qemu-io.c
index 5ef3ef7..4291cb7 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -107,6 +107,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"
+" -L, -- disable image lock\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"
@@ -123,7 +124,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       = "[-rsnLk] [-t cache] [-d discard] [-o options] [path]",
     .oneline    = "open the file specified by path",
     .help       = open_help,
 };
@@ -142,12 +143,13 @@ static int open_f(BlockBackend *blk, int argc, char **argv)
 {
     int flags = BDRV_O_UNMAP;
     int readonly = 0;
+    int nolock = 0;
     bool writethrough = true;
     int c;
     QemuOpts *qopts;
     QDict *opts;
 
-    while ((c = getopt(argc, argv, "snro:kt:d:")) != -1) {
+    while ((c = getopt(argc, argv, "snrLo:kt:d:")) != -1) {
         switch (c) {
         case 's':
             flags |= BDRV_O_SNAPSHOT;
@@ -176,6 +178,9 @@ static int open_f(BlockBackend *blk, int argc, char **argv)
                 return 0;
             }
             break;
+        case 'L':
+            nolock = 1;
+            break;
         case 'o':
             if (imageOpts) {
                 printf("--image-opts and 'open -o' are mutually exclusive\n");
@@ -197,6 +202,10 @@ static int open_f(BlockBackend *blk, int argc, char **argv)
         flags |= BDRV_O_RDWR;
     }
 
+    if (nolock) {
+        flags |= BDRV_O_NO_LOCK;
+    }
+
     if (imageOpts && (optind == argc - 1)) {
         if (!qemu_opts_parse_noisily(&empty_opts, argv[optind], false)) {
             qemu_opts_reset(&empty_opts);
@@ -433,13 +442,15 @@ 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:rLsnmkt:T:";
+    int nolock = 0;
     const struct option lopt[] = {
         { "help", no_argument, NULL, 'h' },
         { "version", no_argument, NULL, 'V' },
         { "cmd", required_argument, NULL, 'c' },
         { "format", required_argument, NULL, 'f' },
         { "read-only", no_argument, NULL, 'r' },
+        { "no-lock", no_argument, NULL, 'L' },
         { "snapshot", no_argument, NULL, 's' },
         { "nocache", no_argument, NULL, 'n' },
         { "misalign", no_argument, NULL, 'm' },
@@ -499,6 +510,9 @@ int main(int argc, char **argv)
         case 'r':
             readonly = 1;
             break;
+        case 'L':
+            nolock = 1;
+            break;
         case 'm':
             qemuio_misalign = true;
             break;
@@ -579,6 +593,10 @@ int main(int argc, char **argv)
         flags |= BDRV_O_RDWR;
     }
 
+    if (nolock) {
+        flags |= BDRV_O_NO_LOCK;
+    }
+
     if ((argc - optind) == 1) {
         if (imageOpts) {
             QemuOpts *qopts = NULL;
-- 
2.8.2

Re: [Qemu-devel] [PATCH v5 13/27] qemu-io: Add "-L" option for BDRV_O_NO_LOCK

[Max Reitz]

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

On 17.05.2016 09:35, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  qemu-io.c | 24 +++++++++++++++++++++---
>  1 file changed, 21 insertions(+), 3 deletions(-)

Looks good overall, but I'd rather use a bool for both nolock variables.

Max


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

[Qemu-devel] [PATCH v5 14/27] qemu-img: Add "-L" option to sub commands

[Fam Zheng]

If specified, BDRV_O_NO_LOCK flag will be set when opening the image.

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

diff --git a/qemu-img.c b/qemu-img.c
index 4792366..b13755b 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -598,6 +598,7 @@ static int img_check(int argc, char **argv)
     ImageCheck *check;
     bool quiet = false;
     bool image_opts = false;
+    bool nolock = false;
 
     fmt = NULL;
     output = NULL;
@@ -614,7 +615,7 @@ static int img_check(int argc, char **argv)
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "hf:r:T:q",
+        c = getopt_long(argc, argv, "hf:r:T:qL",
                         long_options, &option_index);
         if (c == -1) {
             break;
@@ -648,6 +649,9 @@ static int img_check(int argc, char **argv)
         case 'q':
             quiet = true;
             break;
+        case 'L':
+            nolock = true;
+            break;
         case OPTION_OBJECT: {
             QemuOpts *opts;
             opts = qemu_opts_parse_noisily(&qemu_object_opts,
@@ -681,6 +685,7 @@ static int img_check(int argc, char **argv)
         return 1;
     }
 
+    flags |= nolock ? BDRV_O_NO_LOCK : 0;
     ret = bdrv_parse_cache_mode(cache, &flags, &writethrough);
     if (ret < 0) {
         error_report("Invalid source cache option: %s", cache);
@@ -801,6 +806,7 @@ static int img_commit(int argc, char **argv)
     Error *local_err = NULL;
     CommonBlockJobCBInfo cbi;
     bool image_opts = false;
+    bool nolock = false;
 
     fmt = NULL;
     cache = BDRV_DEFAULT_CACHE;
@@ -812,7 +818,7 @@ static int img_commit(int argc, char **argv)
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "f:ht:b:dpq",
+        c = getopt_long(argc, argv, "f:ht:b:dpqL",
                         long_options, NULL);
         if (c == -1) {
             break;
@@ -842,6 +848,9 @@ static int img_commit(int argc, char **argv)
         case 'q':
             quiet = true;
             break;
+        case 'L':
+            nolock = true;
+            break;
         case OPTION_OBJECT: {
             QemuOpts *opts;
             opts = qemu_opts_parse_noisily(&qemu_object_opts,
@@ -873,6 +882,7 @@ static int img_commit(int argc, char **argv)
     }
 
     flags = BDRV_O_RDWR | BDRV_O_UNMAP;
+    flags |= nolock ? BDRV_O_NO_LOCK : 0;
     ret = bdrv_parse_cache_mode(cache, &flags, &writethrough);
     if (ret < 0) {
         error_report("Invalid cache option: %s", cache);
@@ -1131,6 +1141,7 @@ static int img_compare(int argc, char **argv)
     int c, pnum;
     uint64_t progress_base;
     bool image_opts = false;
+    bool nolock = false;
 
     cache = BDRV_DEFAULT_CACHE;
     for (;;) {
@@ -1140,7 +1151,7 @@ static int img_compare(int argc, char **argv)
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "hf:F:T:pqs",
+        c = getopt_long(argc, argv, "hf:F:T:pqsL",
                         long_options, NULL);
         if (c == -1) {
             break;
@@ -1168,6 +1179,9 @@ static int img_compare(int argc, char **argv)
         case 's':
             strict = true;
             break;
+        case 'L':
+            nolock = true;
+            break;
         case OPTION_OBJECT: {
             QemuOpts *opts;
             opts = qemu_opts_parse_noisily(&qemu_object_opts,
@@ -1206,6 +1220,7 @@ static int img_compare(int argc, char **argv)
     qemu_progress_init(progress, 2.0);
 
     flags = 0;
+    flags |= nolock ? BDRV_O_NO_LOCK : 0;
     ret = bdrv_parse_cache_mode(cache, &flags, &writethrough);
     if (ret < 0) {
         error_report("Invalid source cache option: %s", cache);
@@ -1754,6 +1769,7 @@ static int img_convert(int argc, char **argv)
     QemuOpts *sn_opts = NULL;
     ImgConvertState state;
     bool image_opts = false;
+    bool nolock = false;
 
     fmt = NULL;
     out_fmt = "raw";
@@ -1769,7 +1785,7 @@ static int img_convert(int argc, char **argv)
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "hf:O:B:ce6o:s:l:S:pt:T:qn",
+        c = getopt_long(argc, argv, "hf:O:B:ce6o:s:l:S:pt:T:qnL",
                         long_options, NULL);
         if (c == -1) {
             break;
@@ -1858,6 +1874,9 @@ static int img_convert(int argc, char **argv)
         case 'q':
             quiet = true;
             break;
+        case 'L':
+            nolock = true;
+            break;
         case 'n':
             skip_create = 1;
             break;
@@ -1907,6 +1926,7 @@ static int img_convert(int argc, char **argv)
     }
 
     src_flags = 0;
+    src_flags |= nolock ? BDRV_O_NO_LOCK : 0;
     ret = bdrv_parse_cache_mode(src_cache, &src_flags, &src_writethrough);
     if (ret < 0) {
         error_report("Invalid source cache option: %s", src_cache);
@@ -2056,6 +2076,7 @@ static int img_convert(int argc, char **argv)
     }
 
     flags = min_sparse ? (BDRV_O_RDWR | BDRV_O_UNMAP) : BDRV_O_RDWR;
+    flags |= nolock ? BDRV_O_NO_LOCK : 0;
     ret = bdrv_parse_cache_mode(cache, &flags, &writethrough);
     if (ret < 0) {
         error_report("Invalid cache option: %s", cache);
@@ -2236,12 +2257,14 @@ 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 nolock)
 {
     ImageInfoList *head = NULL;
     ImageInfoList **last = &head;
     GHashTable *filenames;
     Error *err = NULL;
+    int flags;
 
     filenames = g_hash_table_new_full(g_str_hash, str_equal_func, NULL, NULL);
 
@@ -2258,8 +2281,9 @@ 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);
+        flags = BDRV_O_NO_BACKING | BDRV_O_NO_IO;
+        flags |= nolock ? BDRV_O_NO_LOCK : 0;
+        blk = img_open(image_opts, filename, fmt, flags, false, false);
         if (!blk) {
             goto err;
         }
@@ -2311,6 +2335,7 @@ static int img_info(int argc, char **argv)
     const char *filename, *fmt, *output;
     ImageInfoList *list;
     bool image_opts = false;
+    bool nolock = false;
 
     fmt = NULL;
     output = NULL;
@@ -2325,7 +2350,7 @@ static int img_info(int argc, char **argv)
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "f:h",
+        c = getopt_long(argc, argv, "f:hL",
                         long_options, &option_index);
         if (c == -1) {
             break;
@@ -2338,6 +2363,9 @@ static int img_info(int argc, char **argv)
         case 'f':
             fmt = optarg;
             break;
+        case 'L':
+            nolock = true;
+            break;
         case OPTION_OUTPUT:
             output = optarg;
             break;
@@ -2377,7 +2405,7 @@ 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, nolock);
     if (!list) {
         return 1;
     }
@@ -2523,6 +2551,8 @@ static int img_map(int argc, char **argv)
     MapEntry curr = { .length = 0 }, next;
     int ret = 0;
     bool image_opts = false;
+    bool nolock = false;
+    int flags;
 
     fmt = NULL;
     output = NULL;
@@ -2536,7 +2566,7 @@ static int img_map(int argc, char **argv)
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "f:h",
+        c = getopt_long(argc, argv, "f:hL",
                         long_options, &option_index);
         if (c == -1) {
             break;
@@ -2549,6 +2579,9 @@ static int img_map(int argc, char **argv)
         case 'f':
             fmt = optarg;
             break;
+        case 'L':
+            nolock = true;
+            break;
         case OPTION_OUTPUT:
             output = optarg;
             break;
@@ -2585,7 +2618,8 @@ static int img_map(int argc, char **argv)
         return 1;
     }
 
-    blk = img_open(image_opts, filename, fmt, 0, false, false);
+    flags = nolock ? BDRV_O_NO_LOCK : 0;
+    blk = img_open(image_opts, filename, fmt, flags, false, false);
     if (!blk) {
         return 1;
     }
@@ -2648,6 +2682,7 @@ static int img_snapshot(int argc, char **argv)
     bool quiet = false;
     Error *err = NULL;
     bool image_opts = false;
+    bool nolock = false;
 
     bdrv_oflags = BDRV_O_RDWR;
     /* Parse commandline parameters */
@@ -2658,7 +2693,7 @@ static int img_snapshot(int argc, char **argv)
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "la:c:d:hq",
+        c = getopt_long(argc, argv, "la:c:d:hqL",
                         long_options, NULL);
         if (c == -1) {
             break;
@@ -2703,6 +2738,9 @@ static int img_snapshot(int argc, char **argv)
         case 'q':
             quiet = true;
             break;
+        case 'L':
+            nolock = true;
+            break;
         case OPTION_OBJECT: {
             QemuOpts *opts;
             opts = qemu_opts_parse_noisily(&qemu_object_opts,
@@ -2728,6 +2766,7 @@ static int img_snapshot(int argc, char **argv)
         return 1;
     }
 
+    bdrv_oflags |= nolock ? BDRV_O_NO_LOCK : 0;
     /* Open the image */
     blk = img_open(image_opts, filename, NULL, bdrv_oflags, false, quiet);
     if (!blk) {
@@ -2797,6 +2836,7 @@ static int img_rebase(int argc, char **argv)
     bool quiet = false;
     Error *local_err = NULL;
     bool image_opts = false;
+    bool nolock = false;
 
     /* Parse commandline parameters */
     fmt = NULL;
@@ -2811,7 +2851,7 @@ static int img_rebase(int argc, char **argv)
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {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:qL",
                         long_options, NULL);
         if (c == -1) {
             break;
@@ -2842,6 +2882,9 @@ static int img_rebase(int argc, char **argv)
         case 'T':
             src_cache = optarg;
             break;
+        case 'L':
+            nolock = true;
+            break;
         case 'q':
             quiet = true;
             break;
@@ -2881,6 +2924,7 @@ static int img_rebase(int argc, char **argv)
     qemu_progress_print(0, 100);
 
     flags = BDRV_O_RDWR | (unsafe ? BDRV_O_NO_BACKING : 0);
+    flags |= nolock ? BDRV_O_NO_LOCK : 0;
     ret = bdrv_parse_cache_mode(cache, &flags, &writethrough);
     if (ret < 0) {
         error_report("Invalid cache option: %s", cache);
@@ -3142,6 +3186,8 @@ static int img_resize(int argc, char **argv)
     bool quiet = false;
     BlockBackend *blk = NULL;
     QemuOpts *param;
+    int flags;
+    bool nolock = false;
 
     static QemuOptsList resize_options = {
         .name = "resize_options",
@@ -3176,7 +3222,7 @@ static int img_resize(int argc, char **argv)
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "f:hq",
+        c = getopt_long(argc, argv, "f:hqL",
                         long_options, NULL);
         if (c == -1) {
             break;
@@ -3192,6 +3238,9 @@ static int img_resize(int argc, char **argv)
         case 'q':
             quiet = true;
             break;
+        case 'L':
+            nolock = true;
+            break;
         case OPTION_OBJECT: {
             QemuOpts *opts;
             opts = qemu_opts_parse_noisily(&qemu_object_opts,
@@ -3243,8 +3292,9 @@ static int img_resize(int argc, char **argv)
     n = qemu_opt_get_size(param, BLOCK_OPT_SIZE, 0);
     qemu_opts_del(param);
 
-    blk = img_open(image_opts, filename, fmt,
-                   BDRV_O_RDWR, false, quiet);
+    flags = BDRV_O_RDWR;
+    flags |= nolock ? BDRV_O_NO_LOCK : 0;
+    blk = img_open(image_opts, filename, fmt, flags, false, quiet);
     if (!blk) {
         ret = -1;
         goto out;
@@ -3305,6 +3355,7 @@ static int img_amend(int argc, char **argv)
     BlockBackend *blk = NULL;
     BlockDriverState *bs = NULL;
     bool image_opts = false;
+    bool nolock = false;
 
     cache = BDRV_DEFAULT_CACHE;
     for (;;) {
@@ -3314,7 +3365,7 @@ static int img_amend(int argc, char **argv)
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "ho:f:t:pq",
+        c = getopt_long(argc, argv, "ho:f:t:pqL",
                         long_options, NULL);
         if (c == -1) {
             break;
@@ -3351,6 +3402,9 @@ static int img_amend(int argc, char **argv)
             case 'q':
                 quiet = true;
                 break;
+            case 'L':
+                nolock = true;
+                break;
             case OPTION_OBJECT:
                 opts = qemu_opts_parse_noisily(&qemu_object_opts,
                                                optarg, true);
@@ -3396,6 +3450,7 @@ static int img_amend(int argc, char **argv)
     }
 
     flags = BDRV_O_RDWR;
+    flags |= nolock ? BDRV_O_NO_LOCK : 0;
     ret = bdrv_parse_cache_mode(cache, &flags, &writethrough);
     if (ret < 0) {
         error_report("Invalid cache option: %s", cache);
-- 
2.8.2

Re: [Qemu-devel] [PATCH v5 14/27] qemu-img: Add "-L" option to sub commands

[Max Reitz]

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

On 17.05.2016 09:35, Fam Zheng wrote:
> If specified, BDRV_O_NO_LOCK flag will be set when opening the image.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  qemu-img.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 72 insertions(+), 17 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 4792366..b13755b 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c

[...]

> @@ -1206,6 +1220,7 @@ static int img_compare(int argc, char **argv)
>      qemu_progress_init(progress, 2.0);
>  
>      flags = 0;
> +    flags |= nolock ? BDRV_O_NO_LOCK : 0;

This reads weird. I'd either put this line below bdrv_parse_cache_mode()
or drop the line initializing src_flags to 0 (and make this a plain
assignment).

>      ret = bdrv_parse_cache_mode(cache, &flags, &writethrough);
>      if (ret < 0) {
>          error_report("Invalid source cache option: %s", cache);

[...]

> @@ -1907,6 +1926,7 @@ static int img_convert(int argc, char **argv)
>      }
>  
>      src_flags = 0;
> +    src_flags |= nolock ? BDRV_O_NO_LOCK : 0;

Same here.

Also: Should we have distinct flags for source and target for convert?
For instance, I can imagine someone wanting not to lock the source but
leave the target in default exclusive mode.

>      ret = bdrv_parse_cache_mode(src_cache, &src_flags, &src_writethrough);
>      if (ret < 0) {
>          error_report("Invalid source cache option: %s", src_cache);

[...]

> @@ -2881,6 +2924,7 @@ static int img_rebase(int argc, char **argv)
>      qemu_progress_print(0, 100);
>  
>      flags = BDRV_O_RDWR | (unsafe ? BDRV_O_NO_BACKING : 0);
> +    flags |= nolock ? BDRV_O_NO_LOCK : 0;
>      ret = bdrv_parse_cache_mode(cache, &flags, &writethrough);
>      if (ret < 0) {
>          error_report("Invalid cache option: %s", cache);

What about the source/base image? And again, would it make sense to have
distinct flags for source and target?

Max


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

Re: [Qemu-devel] [PATCH v5 14/27] qemu-img: Add "-L" option to sub commands

[Fam Zheng]

On Tue, 05/24 20:06, Max Reitz wrote:
> On 17.05.2016 09:35, Fam Zheng wrote:
> > If specified, BDRV_O_NO_LOCK flag will be set when opening the image.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  qemu-img.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++------------
> >  1 file changed, 72 insertions(+), 17 deletions(-)
> > 
> > diff --git a/qemu-img.c b/qemu-img.c
> > index 4792366..b13755b 100644
> > --- a/qemu-img.c
> > +++ b/qemu-img.c
> 
> [...]
> 
> > @@ -1206,6 +1220,7 @@ static int img_compare(int argc, char **argv)
> >      qemu_progress_init(progress, 2.0);
> >  
> >      flags = 0;
> > +    flags |= nolock ? BDRV_O_NO_LOCK : 0;
> 
> This reads weird. I'd either put this line below bdrv_parse_cache_mode()
> or drop the line initializing src_flags to 0 (and make this a plain
> assignment).
> 
> >      ret = bdrv_parse_cache_mode(cache, &flags, &writethrough);
> >      if (ret < 0) {
> >          error_report("Invalid source cache option: %s", cache);
> 
> [...]
> 
> > @@ -1907,6 +1926,7 @@ static int img_convert(int argc, char **argv)
> >      }
> >  
> >      src_flags = 0;
> > +    src_flags |= nolock ? BDRV_O_NO_LOCK : 0;
> 
> Same here.

OK, will drop the dead assignment above.

> 
> Also: Should we have distinct flags for source and target for convert?
> For instance, I can imagine someone wanting not to lock the source but
> leave the target in default exclusive mode.

"-L" is a shorthand flag, for finer control (shared locking mode and separate
modes for src and dest), I would recommend using --image-opts then.

Fam

Re: [Qemu-devel] [PATCH v5 14/27] qemu-img: Add "-L" option to sub commands

[Max Reitz]

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

On 01.06.2016 07:34, Fam Zheng wrote:
> On Tue, 05/24 20:06, Max Reitz wrote:

[...]

>> Also: Should we have distinct flags for source and target for convert?
>> For instance, I can imagine someone wanting not to lock the source but
>> leave the target in default exclusive mode.
> 
> "-L" is a shorthand flag, for finer control (shared locking mode and separate
> modes for src and dest), I would recommend using --image-opts then.

Yes, that's reasonable.

Max


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

[Qemu-devel] [PATCH v5 15/27] qemu-img: Update documentation of "-L" option

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 qemu-img-cmds.hx | 44 ++++++++++++++++++++++----------------------
 qemu-img.c       |  1 +
 qemu-img.texi    |  3 +++
 3 files changed, 26 insertions(+), 22 deletions(-)

diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index e7cded6..fa87942 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -10,68 +10,68 @@ STEXI
 ETEXI
 
 DEF("check", img_check,
-    "check [-q] [--object objectdef] [--image-opts] [-f fmt] [--output=ofmt] [-r [leaks | all]] [-T src_cache] filename")
+    "check [-q] [-L] [--object objectdef] [--image-opts] [-f fmt] [--output=ofmt] [-r [leaks | all]] [-T src_cache] 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] [-L] [-f @var{fmt}] [--output=@var{ofmt}] [-r [leaks | all]] [-T @var{src_cache}] @var{filename}
 ETEXI
 
 DEF("create", img_create,
-    "create [-q] [--object objectdef] [--image-opts] [-f fmt] [-o options] filename [size]")
+    "create [-q] [-L] [--object objectdef] [--image-opts] [-f fmt] [-o options] filename [size]")
 STEXI
-@item create [--object @var{objectdef}] [--image-opts] [-q] [-f @var{fmt}] [-o @var{options}] @var{filename} [@var{size}]
+@item create [--object @var{objectdef}] [--image-opts] [-q] [-L] [-f @var{fmt}] [-o @var{options}] @var{filename} [@var{size}]
 ETEXI
 
 DEF("commit", img_commit,
-    "commit [-q] [--object objectdef] [--image-opts] [-f fmt] [-t cache] [-b base] [-d] [-p] filename")
+    "commit [-q] [-L] [--object objectdef] [--image-opts] [-f fmt] [-t cache] [-b base] [-d] [-p] filename")
 STEXI
-@item commit [--object @var{objectdef}] [--image-opts] [-q] [-f @var{fmt}] [-t @var{cache}] [-b @var{base}] [-d] [-p] @var{filename}
+@item commit [--object @var{objectdef}] [--image-opts] [-q] [-L] [-f @var{fmt}] [-t @var{cache}] [-b @var{base}] [-d] [-p] @var{filename}
 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] [-L] [-s] 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] [-L] [-s] @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] filename [filename2 [...]] output_filename")
+    "convert [--object objectdef] [--image-opts] [-c] [-p] [-q] [-L] [-n] [-f fmt] [-t cache] [-T src_cache] [-O output_fmt] [-o options] [-s snapshot_id_or_name] [-l snapshot_param] [-S sparse_size] 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}] @var{filename} [@var{filename2} [...]] @var{output_filename}
+@item convert [--object @var{objectdef}] [--image-opts] [-c] [-p] [-q] [-L] [-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}] @var{filename} [@var{filename2} [...]] @var{output_filename}
 ETEXI
 
 DEF("info", img_info,
-    "info [--object objectdef] [--image-opts] [-f fmt] [--output=ofmt] [--backing-chain] filename")
+    "info [--object objectdef] [--image-opts] [-f fmt] [-L] [--output=ofmt] [--backing-chain] 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}] [-L] [--output=@var{ofmt}] [--backing-chain] @var{filename}
 ETEXI
 
 DEF("map", img_map,
-    "map [--object objectdef] [--image-opts] [-f fmt] [--output=ofmt] filename")
+    "map [--object objectdef] [--image-opts] [-f fmt] [-L] [--output=ofmt] 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}] [-L] [--output=@var{ofmt}] @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] [-q] [-L] [-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] [-q] [-L] [-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] [-q] [-L] [-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] [-q] [-L] [-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,
-    "resize [--object objectdef] [--image-opts] [-q] filename [+ | -]size")
+    "resize [--object objectdef] [--image-opts] [-q] [-L] filename [+ | -]size")
 STEXI
-@item resize [--object @var{objectdef}] [--image-opts] [-q] @var{filename} [+ | -]@var{size}
+@item resize [--object @var{objectdef}] [--image-opts] [-q] [-L] @var{filename} [+ | -]@var{size}
 ETEXI
 
 DEF("amend", img_amend,
-    "amend [--object objectdef] [--image-opts] [-p] [-q] [-f fmt] [-t cache] -o options filename")
+    "amend [--object objectdef] [--image-opts] [-p] [-q] [-L] [-f fmt] [-t cache] -o options filename")
 STEXI
-@item amend [--object @var{objectdef}] [--image-opts] [-p] [-q] [-f @var{fmt}] [-t @var{cache}] -o @var{options} @var{filename}
+@item amend [--object @var{objectdef}] [--image-opts] [-p] [-q] [-L] [-f @var{fmt}] [-t @var{cache}] -o @var{options} @var{filename}
 @end table
 ETEXI
diff --git a/qemu-img.c b/qemu-img.c
index b13755b..f6738e9 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -130,6 +130,7 @@ static void QEMU_NORETURN help(void)
            "  '-h' with or without a command shows this help and lists the supported formats\n"
            "  '-p' show progress of command (only certain commands)\n"
            "  '-q' use Quiet mode - do not print any output (except errors)\n"
+           "  '-L' disables image locking\n"
            "  '-S' indicates the consecutive number of bytes (defaults to 4k) that must\n"
            "       contain only zeros for qemu-img to create a sparse image during\n"
            "       conversion. If the number of bytes is 0, the source will not be scanned for\n"
diff --git a/qemu-img.texi b/qemu-img.texi
index afaebdd..b91f2e1 100644
--- a/qemu-img.texi
+++ b/qemu-img.texi
@@ -77,6 +77,9 @@ progress is reported when the process receives a @code{SIGUSR1} signal.
 @item -q
 Quiet mode - do not print any output (except errors). There's no progress bar
 in case both @var{-q} and @var{-p} options are used.
+@item -L
+disables image locking. The image will not be locked, other processes can
+access and lock this image while we are using it.
 @item -S @var{size}
 indicates the consecutive number of bytes that must contain only zeros
 for qemu-img to create a sparse image during conversion. This value is rounded
-- 
2.8.2

Re: [Qemu-devel] [PATCH v5 15/27] qemu-img: Update documentation of "-L" option

[Max Reitz]

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

On 17.05.2016 09:35, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  qemu-img-cmds.hx | 44 ++++++++++++++++++++++----------------------
>  qemu-img.c       |  1 +
>  qemu-img.texi    |  3 +++
>  3 files changed, 26 insertions(+), 22 deletions(-)
> 

[...]

> diff --git a/qemu-img.texi b/qemu-img.texi
> index afaebdd..b91f2e1 100644
> --- a/qemu-img.texi
> +++ b/qemu-img.texi
> @@ -77,6 +77,9 @@ progress is reported when the process receives a @code{SIGUSR1} signal.
>  @item -q
>  Quiet mode - do not print any output (except errors). There's no progress bar
>  in case both @var{-q} and @var{-p} options are used.
> +@item -L
> +disables image locking. The image will not be locked, other processes can
> +access and lock this image while we are using it.

I think "while it is in use by qemu-img" sounds more program-y.

Other than that, the patch is OK unless you decide that indeed two
distinct flags for source and target make sense for convert and rebase.

Max

>  @item -S @var{size}
>  indicates the consecutive number of bytes that must contain only zeros
>  for qemu-img to create a sparse image during conversion. This value is rounded
> 



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

[Qemu-devel] [PATCH v5 16/27] qemu-nbd: Add "--no-lock/-L" option

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 qemu-nbd.c    | 7 ++++++-
 qemu-nbd.texi | 2 ++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index 3e54113..57bc18d 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -97,6 +97,7 @@ static void usage(const char *name)
 "Block device options:\n"
 "  -f, --format=FORMAT       set image format (raw, qcow2, ...)\n"
 "  -r, --read-only           export read-only\n"
+"  -L, --no-lock             disable image locking\n"
 "  -s, --snapshot            use FILE as an external snapshot, create a temporary\n"
 "                            file with backing_file=FILE, redirect the write to\n"
 "                            the temporary one\n"
@@ -469,7 +470,7 @@ int main(int argc, char **argv)
     off_t fd_size;
     QemuOpts *sn_opts = NULL;
     const char *sn_id_or_name = NULL;
-    const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:tl:x:";
+    const char *sopt = "hVb:o:p:rsnLP:c:dvk:e:f:tl:x:";
     struct option lopt[] = {
         { "help", no_argument, NULL, 'h' },
         { "version", no_argument, NULL, 'V' },
@@ -478,6 +479,7 @@ int main(int argc, char **argv)
         { "socket", required_argument, NULL, 'k' },
         { "offset", required_argument, NULL, 'o' },
         { "read-only", no_argument, NULL, 'r' },
+        { "no-lock", no_argument, NULL, 'L' },
         { "partition", required_argument, NULL, 'P' },
         { "connect", required_argument, NULL, 'c' },
         { "disconnect", no_argument, NULL, 'd' },
@@ -633,6 +635,9 @@ int main(int argc, char **argv)
             nbdflags |= NBD_FLAG_READ_ONLY;
             flags &= ~BDRV_O_RDWR;
             break;
+        case 'L':
+            flags |= BDRV_O_NO_LOCK;
+            break;
         case 'P':
             partition = strtol(optarg, &end, 0);
             if (*end) {
diff --git a/qemu-nbd.texi b/qemu-nbd.texi
index 9f23343..6b7b1d1 100644
--- a/qemu-nbd.texi
+++ b/qemu-nbd.texi
@@ -43,6 +43,8 @@ Force the use of the block driver for format @var{fmt} instead of
 auto-detecting
 @item -r, --read-only
 Export the disk as read-only
+@item -L, --no-lock
+Disable image locking
 @item -P, --partition=@var{num}
 Only expose partition @var{num}
 @item -s, --snapshot
-- 
2.8.2

Re: [Qemu-devel] [PATCH v5 16/27] qemu-nbd: Add "--no-lock/-L" option

[Max Reitz]

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

On 17.05.2016 09:35, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  qemu-nbd.c    | 7 ++++++-
>  qemu-nbd.texi | 2 ++
>  2 files changed, 8 insertions(+), 1 deletion(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>

Random interjection: You've described -L as "disable image locking", but
as "disables image locking" in the previous patch and as "disable image
lock" in patch 13. Maybe you should settle for a single expression. :-)

Max


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

[Qemu-devel] [PATCH v5 17/27] block: Don't lock drive-backup target image in none mode

[Fam Zheng]

As a very special case, in sync=none mode, the source, as the backing
image of the target, will be RO opened again, which is not accepted by
image locking because the first open could be exclusive.

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

diff --git a/blockdev.c b/blockdev.c
index 0784c4a..250e3d2 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3252,6 +3252,11 @@ static void do_drive_backup(const char *device, const char *target,
         }
     }
     if (sync == MIRROR_SYNC_MODE_NONE) {
+        /* XXX: bs will be open second time as the backing file of target,
+         * disable image locking. Once block layer allows sharing backing BDS,
+         * Change below to BDRV_O_NO_BACKING and assign it after bdrv_open().
+         * */
+        flags |= BDRV_O_NO_LOCK;
         source = bs;
     }
 
-- 
2.8.2

Re: [Qemu-devel] [PATCH v5 17/27] block: Don't lock drive-backup target image in none mode

[Max Reitz]

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

On 17.05.2016 09:35, Fam Zheng wrote:
> As a very special case, in sync=none mode, the source, as the backing
> image of the target, will be RO opened again, which is not accepted by
> image locking because the first open could be exclusive.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  blockdev.c | 5 +++++
>  1 file changed, 5 insertions(+)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

[Qemu-devel] [PATCH v5 18/27] mirror: Disable image locking on target backing chain

[Fam Zheng]

In sync=none the backing image of s->target is s->common.bs, which could
be exclusively locked, the image locking wouldn't work here.

Later we can update completion code to lock it after the replaced node
has dropped its lock.

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

diff --git a/blockdev.c b/blockdev.c
index 250e3d2..3de54f0 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3626,6 +3626,12 @@ void qmp_drive_mirror(const char *device, const char *target,
      * file.
      */
     target_bs = NULL;
+    if (sync == MIRROR_SYNC_MODE_NONE) {
+        flags |= BDRV_O_NO_LOCK;
+    }
+    /* TODO: in mirror complete, after target_bs is switched to and the
+     * original BDS's lock is dropped, we should enable the lock on target_bs.
+     * */
     ret = bdrv_open(&target_bs, target, NULL, options,
                     flags | BDRV_O_NO_BACKING, &local_err);
     if (ret < 0) {
-- 
2.8.2

Re: [Qemu-devel] [PATCH v5 18/27] mirror: Disable image locking on target backing chain

[Max Reitz]

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

On 17.05.2016 09:35, Fam Zheng wrote:
> In sync=none the backing image of s->target is s->common.bs, which could
> be exclusively locked, the image locking wouldn't work here.

Why is the backing image s->common.bs when the bdrv_open() call
explicitly specifies BDRV_O_NO_BACKING?

Max

> Later we can update completion code to lock it after the replaced node
> has dropped its lock.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  blockdev.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 250e3d2..3de54f0 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3626,6 +3626,12 @@ void qmp_drive_mirror(const char *device, const char *target,
>       * file.
>       */
>      target_bs = NULL;
> +    if (sync == MIRROR_SYNC_MODE_NONE) {
> +        flags |= BDRV_O_NO_LOCK;
> +    }
> +    /* TODO: in mirror complete, after target_bs is switched to and the
> +     * original BDS's lock is dropped, we should enable the lock on target_bs.
> +     * */
>      ret = bdrv_open(&target_bs, target, NULL, options,
>                      flags | BDRV_O_NO_BACKING, &local_err);
>      if (ret < 0) {
> 



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

Re: [Qemu-devel] [PATCH v5 18/27] mirror: Disable image locking on target backing chain

[Fam Zheng]

On Tue, 05/24 20:20, Max Reitz wrote:
> On 17.05.2016 09:35, Fam Zheng wrote:
> > In sync=none the backing image of s->target is s->common.bs, which could
> > be exclusively locked, the image locking wouldn't work here.
> 
> Why is the backing image s->common.bs when the bdrv_open() call
> explicitly specifies BDRV_O_NO_BACKING?

BDRV_O_NO_BACKING is cleared in mirror_complete where the backing image is
opened, we cannot use lock there as the backing is already open rw as source.

Fam

> 
> Max
> 
> > Later we can update completion code to lock it after the replaced node
> > has dropped its lock.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  blockdev.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/blockdev.c b/blockdev.c
> > index 250e3d2..3de54f0 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -3626,6 +3626,12 @@ void qmp_drive_mirror(const char *device, const char *target,
> >       * file.
> >       */
> >      target_bs = NULL;
> > +    if (sync == MIRROR_SYNC_MODE_NONE) {
> > +        flags |= BDRV_O_NO_LOCK;
> > +    }
> > +    /* TODO: in mirror complete, after target_bs is switched to and the
> > +     * original BDS's lock is dropped, we should enable the lock on target_bs.
> > +     * */
> >      ret = bdrv_open(&target_bs, target, NULL, options,
> >                      flags | BDRV_O_NO_BACKING, &local_err);
> >      if (ret < 0) {
> > 
> 
> 

Re: [Qemu-devel] [PATCH v5 18/27] mirror: Disable image locking on target backing chain

[Max Reitz]

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

On 03.06.2016 08:32, Fam Zheng wrote:
> On Tue, 05/24 20:20, Max Reitz wrote:
>> On 17.05.2016 09:35, Fam Zheng wrote:
>>> In sync=none the backing image of s->target is s->common.bs, which could
>>> be exclusively locked, the image locking wouldn't work here.
>>
>> Why is the backing image s->common.bs when the bdrv_open() call
>> explicitly specifies BDRV_O_NO_BACKING?
> 
> BDRV_O_NO_BACKING is cleared in mirror_complete where the backing image is
> opened, we cannot use lock there as the backing is already open rw as source.

Oh, yes, of course you're right.

OK, then, if patch 7 of this series worked correctly, the bdrv_reopen()
in mirror_exit() would copy the locking status from the source BDS to
the target, effectively again overriding the disabled locking as it has
been set here.

I think the real issue is that mirror_complete() opens a completely new
backing chain, when it could just reuse the existing backing chain of
the source BDS. Unless you want to do that yourself, I guess I can take
a look into that, as this is very much related to my "block/mirror: Fix
target backing BDS" series.

Max


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

[Qemu-devel] [PATCH v5 19/27] qemu-iotests: 140: Disable image lock for qemu-io access

[Fam Zheng]

The VM is still on, the image locking check would complain.

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

diff --git a/tests/qemu-iotests/140 b/tests/qemu-iotests/140
index 49f9df4..3be656a 100755
--- a/tests/qemu-iotests/140
+++ b/tests/qemu-iotests/140
@@ -70,7 +70,7 @@ _send_qemu_cmd $QEMU_HANDLE \
        'arguments': { 'device': 'drv' }}" \
     'return'
 
-$QEMU_IO_PROG -f raw -c 'read -P 42 0 64k' \
+$QEMU_IO_PROG -f raw -L -c 'read -P 42 0 64k' \
     "nbd+unix:///drv?socket=$TEST_DIR/nbd" 2>&1 \
     | _filter_qemu_io | _filter_nbd
 
-- 
2.8.2

Re: [Qemu-devel] [PATCH v5 19/27] qemu-iotests: 140: Disable image lock for qemu-io access

[Max Reitz]

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

On 17.05.2016 09:35, Fam Zheng wrote:
> The VM is still on, the image locking check would complain.

No, it wouldn't. We are opening an NBD URL that is provided by the VM.

Even if it would, we are only reading data, so an -r would probably be
better than an -L.

Max

> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  tests/qemu-iotests/140 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/qemu-iotests/140 b/tests/qemu-iotests/140
> index 49f9df4..3be656a 100755
> --- a/tests/qemu-iotests/140
> +++ b/tests/qemu-iotests/140
> @@ -70,7 +70,7 @@ _send_qemu_cmd $QEMU_HANDLE \
>         'arguments': { 'device': 'drv' }}" \
>      'return'
>  
> -$QEMU_IO_PROG -f raw -c 'read -P 42 0 64k' \
> +$QEMU_IO_PROG -f raw -L -c 'read -P 42 0 64k' \
>      "nbd+unix:///drv?socket=$TEST_DIR/nbd" 2>&1 \
>      | _filter_qemu_io | _filter_nbd
>  
> 



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

Re: [Qemu-devel] [PATCH v5 19/27] qemu-iotests: 140: Disable image lock for qemu-io access

[Fam Zheng]

On Wed, 05/25 15:16, Max Reitz wrote:
> On 17.05.2016 09:35, Fam Zheng wrote:
> > The VM is still on, the image locking check would complain.
> 
> No, it wouldn't. We are opening an NBD URL that is provided by the VM.
> 
> Even if it would, we are only reading data, so an -r would probably be
> better than an -L.

You are right, I am dropping this patch.

Fam

> 
> Max
> 
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  tests/qemu-iotests/140 | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tests/qemu-iotests/140 b/tests/qemu-iotests/140
> > index 49f9df4..3be656a 100755
> > --- a/tests/qemu-iotests/140
> > +++ b/tests/qemu-iotests/140
> > @@ -70,7 +70,7 @@ _send_qemu_cmd $QEMU_HANDLE \
> >         'arguments': { 'device': 'drv' }}" \
> >      'return'
> >  
> > -$QEMU_IO_PROG -f raw -c 'read -P 42 0 64k' \
> > +$QEMU_IO_PROG -f raw -L -c 'read -P 42 0 64k' \
> >      "nbd+unix:///drv?socket=$TEST_DIR/nbd" 2>&1 \
> >      | _filter_qemu_io | _filter_nbd
> >  
> > 
> 
> 

[Qemu-devel] [PATCH v5 20/27] qemu-iotests: 046: Move version detection out from verify_io

[Fam Zheng]

So the image lock won't complain.

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

diff --git a/tests/qemu-iotests/046 b/tests/qemu-iotests/046
index e528b67..365658e 100755
--- a/tests/qemu-iotests/046
+++ b/tests/qemu-iotests/046
@@ -192,15 +192,7 @@ echo "== Verify image content =="
 
 function verify_io()
 {
-    if ($QEMU_IMG info -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
-        discarded=
-    else
-        # Discarded clusters are zeroed for v3 or later
-        discarded=0
-    fi
+    discarded=$1
 
     echo read -P 0 0 0x10000
 
@@ -261,7 +253,17 @@ function verify_io()
     echo read -P 17  0x11c000 0x4000
 }
 
-verify_io | $QEMU_IO "$TEST_IMG" | _filter_qemu_io
+if ($QEMU_IMG info -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
+    discarded=
+else
+    # Discarded clusters are zeroed for v3 or later
+    discarded=0
+fi
+
+verify_io $discarded | $QEMU_IO -L "$TEST_IMG" | _filter_qemu_io
 
 _check_test_img
 
-- 
2.8.2

Re: [Qemu-devel] [PATCH v5 20/27] qemu-iotests: 046: Move version detection out from verify_io

[Max Reitz]

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

On 17.05.2016 09:35, Fam Zheng wrote:
> So the image lock won't complain.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  tests/qemu-iotests/046 | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/tests/qemu-iotests/046 b/tests/qemu-iotests/046
> index e528b67..365658e 100755
> --- a/tests/qemu-iotests/046
> +++ b/tests/qemu-iotests/046
> @@ -192,15 +192,7 @@ echo "== Verify image content =="
>  
>  function verify_io()
>  {
> -    if ($QEMU_IMG info -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
> -        discarded=
> -    else
> -        # Discarded clusters are zeroed for v3 or later
> -        discarded=0
> -    fi
> +    discarded=$1
>  
>      echo read -P 0 0 0x10000
>  
> @@ -261,7 +253,17 @@ function verify_io()
>      echo read -P 17  0x11c000 0x4000
>  }
>  
> -verify_io | $QEMU_IO "$TEST_IMG" | _filter_qemu_io
> +if ($QEMU_IMG info -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
> +    discarded=
> +else
> +    # Discarded clusters are zeroed for v3 or later
> +    discarded=0
> +fi
> +
> +verify_io $discarded | $QEMU_IO -L "$TEST_IMG" | _filter_qemu_io

With the code movement, the -L becomes unnecessary here.

Max

>  
>  _check_test_img
>  
> 



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

Re: [Qemu-devel] [PATCH v5 20/27] qemu-iotests: 046: Move version detection out from verify_io

[Fam Zheng]

On Wed, 05/25 15:23, Max Reitz wrote:
> > -verify_io | $QEMU_IO "$TEST_IMG" | _filter_qemu_io
> > +if ($QEMU_IMG info -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
> > +    discarded=
> > +else
> > +    # Discarded clusters are zeroed for v3 or later
> > +    discarded=0
> > +fi
> > +
> > +verify_io $discarded | $QEMU_IO -L "$TEST_IMG" | _filter_qemu_io
> 
> With the code movement, the -L becomes unnecessary here.

Good point, removing it.

Fam

[Qemu-devel] [PATCH v5 21/27] qemu-iotests: Wait for QEMU processes before checking image in 091

[Fam Zheng]

We should wait for the QEMU process to terminate and close the image
before we check the data.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 tests/qemu-iotests/091     | 3 +++
 tests/qemu-iotests/091.out | 1 +
 2 files changed, 4 insertions(+)

diff --git a/tests/qemu-iotests/091 b/tests/qemu-iotests/091
index 32bbd56..32aa5c3 100755
--- a/tests/qemu-iotests/091
+++ b/tests/qemu-iotests/091
@@ -95,6 +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' ""
+echo "vm1: 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
diff --git a/tests/qemu-iotests/091.out b/tests/qemu-iotests/091.out
index 5017f8c..6658ca8 100644
--- a/tests/qemu-iotests/091.out
+++ b/tests/qemu-iotests/091.out
@@ -18,6 +18,7 @@ vm1: live migration completed
 vm2: qemu-io disk write complete
 vm2: qemu process running successfully
 vm2: flush io, and quit
+vm1: 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)
-- 
2.8.2

Re: [Qemu-devel] [PATCH v5 21/27] qemu-iotests: Wait for QEMU processes before checking image in 091

[Max Reitz]

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

On 17.05.2016 09:35, Fam Zheng wrote:
> We should wait for the QEMU process to terminate and close the image
> before we check the data.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  tests/qemu-iotests/091     | 3 +++
>  tests/qemu-iotests/091.out | 1 +
>  2 files changed, 4 insertions(+)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

[Qemu-devel] [PATCH v5 22/27] qemu-iotests: 030: Disable image lock when checking test image

[Fam Zheng]

The VM is running, qemu-io would fail the lock acquisition.

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

diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index 3ac2443..fa996ef 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -95,7 +95,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('-L', '-f', iotests.imgfmt, '-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)
-- 
2.8.2

Re: [Qemu-devel] [PATCH v5 22/27] qemu-iotests: 030: Disable image lock when checking test image

[Max Reitz]

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

On 17.05.2016 09:35, Fam Zheng wrote:
> The VM is running, qemu-io would fail the lock acquisition.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  tests/qemu-iotests/030 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

[Qemu-devel] [PATCH v5 23/27] iotests: 087: Disable image lock in cases where file is shared

[Fam Zheng]

Otherwise the error handling we are expceting will be masked by the
preceding image locking check, and is going to be indistinguishable
because the error messages are all the same.

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

diff --git a/tests/qemu-iotests/087 b/tests/qemu-iotests/087
index e7bca37..60bdec5 100755
--- a/tests/qemu-iotests/087
+++ b/tests/qemu-iotests/087
@@ -84,6 +84,7 @@ run_qemu <<EOF
       "options": {
         "driver": "$IMGFMT",
         "id": "disk",
+        "lock-mode": "off",
         "node-name": "test-node",
         "file": {
             "driver": "file",
@@ -97,6 +98,7 @@ run_qemu <<EOF
       "options": {
         "driver": "$IMGFMT",
         "id": "disk",
+        "lock-mode": "off",
         "file": {
             "driver": "file",
             "filename": "$TEST_IMG"
@@ -109,6 +111,7 @@ run_qemu <<EOF
       "options": {
         "driver": "$IMGFMT",
         "id": "test-node",
+        "lock-mode": "off",
         "file": {
             "driver": "file",
             "filename": "$TEST_IMG"
@@ -122,6 +125,7 @@ run_qemu <<EOF
         "driver": "$IMGFMT",
         "id": "disk2",
         "node-name": "disk",
+        "lock-mode": "off",
         "file": {
             "driver": "file",
             "filename": "$TEST_IMG"
@@ -135,6 +139,7 @@ run_qemu <<EOF
         "driver": "$IMGFMT",
         "id": "disk2",
         "node-name": "test-node",
+        "lock-mode": "off",
         "file": {
             "driver": "file",
             "filename": "$TEST_IMG"
@@ -148,6 +153,7 @@ run_qemu <<EOF
         "driver": "$IMGFMT",
         "id": "disk3",
         "node-name": "disk3",
+        "lock-mode": "off",
         "file": {
             "driver": "file",
             "filename": "$TEST_IMG"
-- 
2.8.2

Re: [Qemu-devel] [PATCH v5 23/27] iotests: 087: Disable image lock in cases where file is shared

[Max Reitz]

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

On 17.05.2016 09:35, Fam Zheng wrote:
> Otherwise the error handling we are expceting will be masked by the

*expecting

> preceding image locking check, and is going to be indistinguishable
> because the error messages are all the same.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  tests/qemu-iotests/087 | 6 ++++++
>  1 file changed, 6 insertions(+)

With the spelling error fixed:

Reviewed-by: Max Reitz <mreitz@redhat.com>

But maybe it would be easier to just make these test cases use null-co
instead of a qcow2 file.

> 
> diff --git a/tests/qemu-iotests/087 b/tests/qemu-iotests/087
> index e7bca37..60bdec5 100755
> --- a/tests/qemu-iotests/087
> +++ b/tests/qemu-iotests/087
> @@ -84,6 +84,7 @@ run_qemu <<EOF
>        "options": {
>          "driver": "$IMGFMT",
>          "id": "disk",
> +        "lock-mode": "off",
>          "node-name": "test-node",

And maybe you want to move the lock-mode below the node-name here,
because that's where you put it everywhere else in this patch.

Max

>          "file": {
>              "driver": "file",
> @@ -97,6 +98,7 @@ run_qemu <<EOF
>        "options": {
>          "driver": "$IMGFMT",
>          "id": "disk",
> +        "lock-mode": "off",
>          "file": {
>              "driver": "file",
>              "filename": "$TEST_IMG"
> @@ -109,6 +111,7 @@ run_qemu <<EOF
>        "options": {
>          "driver": "$IMGFMT",
>          "id": "test-node",
> +        "lock-mode": "off",
>          "file": {
>              "driver": "file",
>              "filename": "$TEST_IMG"
> @@ -122,6 +125,7 @@ run_qemu <<EOF
>          "driver": "$IMGFMT",
>          "id": "disk2",
>          "node-name": "disk",
> +        "lock-mode": "off",
>          "file": {
>              "driver": "file",
>              "filename": "$TEST_IMG"
> @@ -135,6 +139,7 @@ run_qemu <<EOF
>          "driver": "$IMGFMT",
>          "id": "disk2",
>          "node-name": "test-node",
> +        "lock-mode": "off",
>          "file": {
>              "driver": "file",
>              "filename": "$TEST_IMG"
> @@ -148,6 +153,7 @@ run_qemu <<EOF
>          "driver": "$IMGFMT",
>          "id": "disk3",
>          "node-name": "disk3",
> +        "lock-mode": "off",
>          "file": {
>              "driver": "file",
>              "filename": "$TEST_IMG"
> 



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

Re: [Qemu-devel] [PATCH v5 23/27] iotests: 087: Disable image lock in cases where file is shared

[Max Reitz]

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

On 17.05.2016 09:35, Fam Zheng wrote:
> Otherwise the error handling we are expceting will be masked by the

*expecting

> preceding image locking check, and is going to be indistinguishable
> because the error messages are all the same.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  tests/qemu-iotests/087 | 6 ++++++
>  1 file changed, 6 insertions(+)

With the spelling error fixed:

Reviewed-by: Max Reitz <mreitz@redhat.com>

But maybe it would be easier to just make these test cases use null-co
instead of a $IMGFMT file.

> 
> diff --git a/tests/qemu-iotests/087 b/tests/qemu-iotests/087
> index e7bca37..60bdec5 100755
> --- a/tests/qemu-iotests/087
> +++ b/tests/qemu-iotests/087
> @@ -84,6 +84,7 @@ run_qemu <<EOF
>        "options": {
>          "driver": "$IMGFMT",
>          "id": "disk",
> +        "lock-mode": "off",
>          "node-name": "test-node",

And maybe you want to move the lock-mode below the node-name here,
because that's where you put it everywhere else in this patch.

Max

>          "file": {
>              "driver": "file",
> @@ -97,6 +98,7 @@ run_qemu <<EOF
>        "options": {
>          "driver": "$IMGFMT",
>          "id": "disk",
> +        "lock-mode": "off",
>          "file": {
>              "driver": "file",
>              "filename": "$TEST_IMG"
> @@ -109,6 +111,7 @@ run_qemu <<EOF
>        "options": {
>          "driver": "$IMGFMT",
>          "id": "test-node",
> +        "lock-mode": "off",
>          "file": {
>              "driver": "file",
>              "filename": "$TEST_IMG"
> @@ -122,6 +125,7 @@ run_qemu <<EOF
>          "driver": "$IMGFMT",
>          "id": "disk2",
>          "node-name": "disk",
> +        "lock-mode": "off",
>          "file": {
>              "driver": "file",
>              "filename": "$TEST_IMG"
> @@ -135,6 +139,7 @@ run_qemu <<EOF
>          "driver": "$IMGFMT",
>          "id": "disk2",
>          "node-name": "test-node",
> +        "lock-mode": "off",
>          "file": {
>              "driver": "file",
>              "filename": "$TEST_IMG"
> @@ -148,6 +153,7 @@ run_qemu <<EOF
>          "driver": "$IMGFMT",
>          "id": "disk3",
>          "node-name": "disk3",
> +        "lock-mode": "off",
>          "file": {
>              "driver": "file",
>              "filename": "$TEST_IMG"
> 



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

[Qemu-devel] [PATCH v5 24/27] iotests: Disable image locking in 085

[Fam Zheng]

The cases is about live snapshot features. Disable image locking because
otherwise a few tests are going to fail because we reuse the same images
at blockdev-add.

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

diff --git a/tests/qemu-iotests/085 b/tests/qemu-iotests/085
index aa77eca..48f6684 100755
--- a/tests/qemu-iotests/085
+++ b/tests/qemu-iotests/085
@@ -102,6 +102,7 @@ function add_snapshot_image()
     cmd="{ 'execute': 'blockdev-add', 'arguments':
            { 'options':
              { 'driver': 'qcow2', 'node-name': 'snap_${1}', ${extra_params}
+               'lock-mode': 'off',
                'file':
                { 'driver': 'file', 'filename': '${snapshot_file}',
                  'node-name': 'file_${1}' } } } }"
@@ -130,7 +131,7 @@ echo === Running QEMU ===
 echo
 
 qemu_comm_method="qmp"
-_launch_qemu -drive file="${TEST_IMG}.1",if=virtio -drive file="${TEST_IMG}.2",if=virtio
+_launch_qemu -drive file="${TEST_IMG}.1",if=virtio,lock-mode=off -drive file="${TEST_IMG}.2",if=virtio,lock-mode=off
 h=$QEMU_HANDLE
 
 echo
-- 
2.8.2

Re: [Qemu-devel] [PATCH v5 24/27] iotests: Disable image locking in 085

[Max Reitz]

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

On 17.05.2016 09:35, Fam Zheng wrote:
> The cases is about live snapshot features. Disable image locking because
> otherwise a few tests are going to fail because we reuse the same images
> at blockdev-add.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  tests/qemu-iotests/085 | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/qemu-iotests/085 b/tests/qemu-iotests/085
> index aa77eca..48f6684 100755
> --- a/tests/qemu-iotests/085
> +++ b/tests/qemu-iotests/085
> @@ -102,6 +102,7 @@ function add_snapshot_image()
>      cmd="{ 'execute': 'blockdev-add', 'arguments':
>             { 'options':
>               { 'driver': 'qcow2', 'node-name': 'snap_${1}', ${extra_params}
> +               'lock-mode': 'off',
>                 'file':
>                 { 'driver': 'file', 'filename': '${snapshot_file}',
>                   'node-name': 'file_${1}' } } } }"
> @@ -130,7 +131,7 @@ echo === Running QEMU ===
>  echo
>  
>  qemu_comm_method="qmp"
> -_launch_qemu -drive file="${TEST_IMG}.1",if=virtio -drive file="${TEST_IMG}.2",if=virtio
> +_launch_qemu -drive file="${TEST_IMG}.1",if=virtio,lock-mode=off -drive file="${TEST_IMG}.2",if=virtio,lock-mode=off
>  h=$QEMU_HANDLE
>  
>  echo
> 

So as far as I understand it, add_snapshot_image() is supposed to add
images from the backing chain to the running VM. The top image is never
used by add_snapshot_image(), thus the lock-mode=off in the QEMU command
line seems superfluous.

Since the backing chain is opened read-only by the VM, it is locked in
shared mode, basically. Therefore, we can simply use explicitly shared
lock mode in add_snapshot_image(); or, alternatively, it is completely
sufficient to specify "'read-only': true" there instead of forcing a
non-exclusive locking mode.

And indeed, for me the test passes if I undo the changes done by this
patch and just insert said "'read-only': true" in the blockdev-add
invocation done by add_snapshot_image().

Max


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

Re: [Qemu-devel] [PATCH v5 24/27] iotests: Disable image locking in 085

[Fam Zheng]

On Wed, 05/25 15:52, Max Reitz wrote:
> On 17.05.2016 09:35, Fam Zheng wrote:
> > The cases is about live snapshot features. Disable image locking because
> > otherwise a few tests are going to fail because we reuse the same images
> > at blockdev-add.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  tests/qemu-iotests/085 | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tests/qemu-iotests/085 b/tests/qemu-iotests/085
> > index aa77eca..48f6684 100755
> > --- a/tests/qemu-iotests/085
> > +++ b/tests/qemu-iotests/085
> > @@ -102,6 +102,7 @@ function add_snapshot_image()
> >      cmd="{ 'execute': 'blockdev-add', 'arguments':
> >             { 'options':
> >               { 'driver': 'qcow2', 'node-name': 'snap_${1}', ${extra_params}
> > +               'lock-mode': 'off',
> >                 'file':
> >                 { 'driver': 'file', 'filename': '${snapshot_file}',
> >                   'node-name': 'file_${1}' } } } }"
> > @@ -130,7 +131,7 @@ echo === Running QEMU ===
> >  echo
> >  
> >  qemu_comm_method="qmp"
> > -_launch_qemu -drive file="${TEST_IMG}.1",if=virtio -drive file="${TEST_IMG}.2",if=virtio
> > +_launch_qemu -drive file="${TEST_IMG}.1",if=virtio,lock-mode=off -drive file="${TEST_IMG}.2",if=virtio,lock-mode=off
> >  h=$QEMU_HANDLE
> >  
> >  echo
> > 
> 
> So as far as I understand it, add_snapshot_image() is supposed to add
> images from the backing chain to the running VM. The top image is never
> used by add_snapshot_image(), thus the lock-mode=off in the QEMU command
> line seems superfluous.

But down the backing chain is 10-snapshot-v0.qcow2, created in
create_single_snapshot (or create_group_snapshot?). Without lock-mode=off in
the command line, the shared lock cannot work.

> 
> Since the backing chain is opened read-only by the VM, it is locked in
> shared mode, basically. Therefore, we can simply use explicitly shared
> lock mode in add_snapshot_image(); or, alternatively, it is completely
> sufficient to specify "'read-only': true" there instead of forcing a
> non-exclusive locking mode.
> 
> And indeed, for me the test passes if I undo the changes done by this
> patch and just insert said "'read-only': true" in the blockdev-add
> invocation done by add_snapshot_image().

For above reason, this doesn't work for me. Am I missing anything?

Fam

Re: [Qemu-devel] [PATCH v5 24/27] iotests: Disable image locking in 085

[Max Reitz]

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

On 03.06.2016 09:18, Fam Zheng wrote:
> On Wed, 05/25 15:52, Max Reitz wrote:
>> On 17.05.2016 09:35, Fam Zheng wrote:
>>> The cases is about live snapshot features. Disable image locking because
>>> otherwise a few tests are going to fail because we reuse the same images
>>> at blockdev-add.
>>>
>>> Signed-off-by: Fam Zheng <famz@redhat.com>
>>> ---
>>>  tests/qemu-iotests/085 | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tests/qemu-iotests/085 b/tests/qemu-iotests/085
>>> index aa77eca..48f6684 100755
>>> --- a/tests/qemu-iotests/085
>>> +++ b/tests/qemu-iotests/085
>>> @@ -102,6 +102,7 @@ function add_snapshot_image()
>>>      cmd="{ 'execute': 'blockdev-add', 'arguments':
>>>             { 'options':
>>>               { 'driver': 'qcow2', 'node-name': 'snap_${1}', ${extra_params}
>>> +               'lock-mode': 'off',
>>>                 'file':
>>>                 { 'driver': 'file', 'filename': '${snapshot_file}',
>>>                   'node-name': 'file_${1}' } } } }"
>>> @@ -130,7 +131,7 @@ echo === Running QEMU ===
>>>  echo
>>>  
>>>  qemu_comm_method="qmp"
>>> -_launch_qemu -drive file="${TEST_IMG}.1",if=virtio -drive file="${TEST_IMG}.2",if=virtio
>>> +_launch_qemu -drive file="${TEST_IMG}.1",if=virtio,lock-mode=off -drive file="${TEST_IMG}.2",if=virtio,lock-mode=off
>>>  h=$QEMU_HANDLE
>>>  
>>>  echo
>>>
>>
>> So as far as I understand it, add_snapshot_image() is supposed to add
>> images from the backing chain to the running VM. The top image is never
>> used by add_snapshot_image(), thus the lock-mode=off in the QEMU command
>> line seems superfluous.
> 
> But down the backing chain is 10-snapshot-v0.qcow2, created in
> create_single_snapshot (or create_group_snapshot?). Without lock-mode=off in
> the command line, the shared lock cannot work.

My reasoning was wrong, but still it works for me if the lock-mode is
omitted everywhere but the blockdev-add operation specifies
"'read-only': true".

OK, so let's try again.

<what_the_read_only_does_and_where_to_put_it>
<explanation_what_happens>

The files specified on the command line will later be snapshotted; the
snapshot operation will reopen them read-only, therefore, they will have
a shared lock then.

Only after those snapshots have occurred will we blockdev-add new files.
If we do not force them to be opened read-only, the "Invalid command -
snapshot node has a backing image" test will fail. This is because of
the following (I believe):

This test is the only one that adds a new snapshot image with its
backing chain (of which some images have already been opened by qemu).
Of course, the whole backing chain is opened read-only, therefore this
is only an issue if some image of that chain has been opened read/write
by qemu before.

Now, blockdev-snapshot-sync will keep the flags of the old BDS for the
new snapshot BDS. Therefore, if we made the original drive read-only,
all snapshots created with that command would remain read-only and no
locking issue would occur.

However, we are sometimes using blockdev-snapshot, and that command of
course will not change the flags the existing overlay node has been
created with. If we are therefore not making these nodes read-only, the
images opened for virtio0 and virtio1 will not be read-only at the time
of the "Invalid command - snapshot node has a backing image" test.

</explanation_what_happens>

If we specify "'read-only': true" in the blockdev-add command, they will
be read-only, on the other hand. Then, the locking will not interfere.

For the sake of clarity, I would then however propose specifying
read-only on the command line, too, so it is clear that we are trying to
keep the drives read-only the entire time.

</what_the_read_only_does_and_where_to_put_it>


But this is a bit complicated, and rather complicated to understand,
too. The issue we really have is that that one test tries to open a
snapshot with its backing chain while parts of the backing chain are
still opened by qemu. Therefore, alternatively, we can just switch off
locking in that single case. This can be accomplished by replacing

extra_params=""

by

extra_params="'lock-mode': 'off', "

in add_snapshot_image (second line in that function's body). That works
for me.

Also, I guess I made us waste too much time on this already... If this
doesn't work for you, I'm fine with the original patch as it was.

Max


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

[Qemu-devel] [PATCH v5 25/27] tests: Use null-co:// instead of /dev/null

[Fam Zheng]

With image locking, opening /dev/null can fail when multiple tests run
in parallel (make -j2, for example). Use null-co:// as the null protocol
doesn't do image locking.

While it's arguable we could special-case /dev/null, /dev/zero,
/dev/urandom etc in raw-posix driver, it is not really necessary because
user can always specify lock-mode=off when it is appropriate. So let's
write sensible testing code too.

Signed-off-by: Fam Zheng <famz@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  | 4 ++--
 6 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/tests/drive_del-test.c b/tests/drive_del-test.c
index fe03236..b9a6036 100644
--- a/tests/drive_del-test.c
+++ b/tests/drive_del-test.c
@@ -93,7 +93,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 ec06893..60734e8 100644
--- a/tests/nvme-test.c
+++ b/tests/nvme-test.c
@@ -23,7 +23,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 71ff2ea..4a2f3ad 100644
--- a/tests/usb-hcd-uhci-test.c
+++ b/tests/usb-hcd-uhci-test.c
@@ -86,7 +86,7 @@ int main(int argc, char **argv)
     qtest_add_func("/uhci/pci/hotplug/usb-storage", test_usb_storage_hotplug);
 
     qtest_start("-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");
     ret = g_test_run();
     qtest_end();
diff --git a/tests/usb-hcd-xhci-test.c b/tests/usb-hcd-xhci-test.c
index 7e2e212..5e96bec 100644
--- a/tests/usb-hcd-xhci-test.c
+++ b/tests/usb-hcd-xhci-test.c
@@ -90,7 +90,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 3a66630..7c21938 100644
--- a/tests/virtio-blk-test.c
+++ b/tests/virtio-blk-test.c
@@ -81,7 +81,7 @@ static QPCIBus *pci_test_start(void)
     tmp_path = drive_create();
 
     cmdline = g_strdup_printf("-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",
                         tmp_path, PCI_SLOT, PCI_FN);
diff --git a/tests/virtio-scsi-test.c b/tests/virtio-scsi-test.c
index d78747a..74f85a8 100644
--- a/tests/virtio-scsi-test.c
+++ b/tests/virtio-scsi-test.c
@@ -57,7 +57,7 @@ static void qvirtio_scsi_start(const char *extra_opts)
     char *cmdline;
 
     cmdline = g_strdup_printf(
-                "-drive id=drv0,if=none,file=/dev/null,format=raw "
+                "-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",
                 extra_opts ? : "");
@@ -208,7 +208,7 @@ static void hotplug(void)
 {
     QDict *response;
 
-    qvirtio_scsi_start("-drive id=drv1,if=none,file=/dev/null,format=raw");
+    qvirtio_scsi_start("-drive id=drv1,if=none,file=null-co://,format=raw");
     response = qmp("{\"execute\": \"device_add\","
                    " \"arguments\": {"
                    "   \"driver\": \"scsi-hd\","
-- 
2.8.2

Re: [Qemu-devel] [PATCH v5 25/27] tests: Use null-co:// instead of /dev/null

[Max Reitz]

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

On 17.05.2016 09:35, Fam Zheng wrote:
> With image locking, opening /dev/null can fail when multiple tests run
> in parallel (make -j2, for example). Use null-co:// as the null protocol
> doesn't do image locking.
> 
> While it's arguable we could special-case /dev/null, /dev/zero,
> /dev/urandom etc in raw-posix driver, it is not really necessary because
> user can always specify lock-mode=off when it is appropriate. So let's
> write sensible testing code too.
> 
> Signed-off-by: Fam Zheng <famz@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  | 4 ++--
>  6 files changed, 7 insertions(+), 7 deletions(-)

I prefer a plain driver=null-co instead of file=null-co://,format=raw,
but whatever floats your boat.

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

[Qemu-devel] [PATCH v5 26/27] block: Turn on image locking by default

[Fam Zheng]

Now that test cases are covered, we can turn it on.

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

diff --git a/blockdev.c b/blockdev.c
index 3de54f0..cd72597 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -384,7 +384,7 @@ static void extract_common_blockdev_options(QemuOpts *opts, int *bdrv_flags,
             }
         }
 
-        lock_mode = qemu_opt_get(opts, "lock-mode") ? : "off";
+        lock_mode = qemu_opt_get(opts, "lock-mode") ? : "exclusive";
         if (!strcmp(lock_mode, "exclusive")) {
             /* Default */
         } else if (!strcmp(lock_mode, "shared")) {
-- 
2.8.2

Re: [Qemu-devel] [PATCH v5 26/27] block: Turn on image locking by default

[Max Reitz]

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

On 17.05.2016 09:35, Fam Zheng wrote:
> Now that test cases are covered, we can turn it on.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  blockdev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

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

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 tests/qemu-iotests/153     | 197 +++++++++++++++++++++
 tests/qemu-iotests/153.out | 426 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 624 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..cbeda8e
--- /dev/null
+++ b/tests/qemu-iotests/153
@@ -0,0 +1,197 @@
+#!/bin/bash
+#
+# Test image locking
+#
+# Copyright (C) 2016 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
+}
+
+for opts1 in "" "readonly=on" "lock-mode=off" "lock-mode=shared"; 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 $opts1 =="
+    _launch_qemu -drive file="${TEST_IMG}",if=none,$opts1
+    h=$QEMU_HANDLE
+
+    for opts2 in "" "lock-mode=exclusive" "lock-mode=shared" \
+                 "lock-mode=off" "lock-mode=exclusive,readonly=on"; do
+        echo
+        echo "== Launching another QEMU $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 "" "-L"; do
+
+        echo
+        echo "== Running utility commands $(test -n "$L" && echo "(lock disabled)") =="
+        _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_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"
+    done
+    _send_qemu_cmd $h "{ 'execute': 'quit', }" ""
+    echo
+    echo "Round done"
+    _cleanup_qemu
+done
+
+function _enum_opts()
+{
+    echo lock-mode={shared,exclusive,off},readonly={on,off}
+}
+
+for opt1 in $(_enum_opts); do
+    for opt2 in $(_enum_opts); do
+        echo
+        echo "== Two devices with the same image ($opt1 - $opt2) =="
+        _run_qemu_with_images "${TEST_IMG},$opt1" "${TEST_IMG},$opt2"
+    done
+done
+
+(
+    $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 "== Backing image also as an active device (shared) =="
+_run_qemu_with_images "${TEST_IMG}.a" "${TEST_IMG},lock-mode=shared"
+
+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 "== Test that close one BDS doesn't unlock others =="
+_launch_qemu -drive file="${TEST_IMG}",if=none,id=d0
+
+_send_qemu_cmd $QEMU_HANDLE \
+    "{ 'execute': 'qmp_capabilities' }" \
+    'return'
+
+_send_qemu_cmd $QEMU_HANDLE \
+    "{ 'execute': 'human-monitor-command',
+       'arguments': { 'command-line': 'drive_add 0 if=none,id=d1,file=${TEST_IMG}' } }" \
+    "Failed to lock image"
+
+_send_qemu_cmd $QEMU_HANDLE \
+    "{ 'execute': 'human-monitor-command',
+       'arguments': { 'command-line': 'drive_add 0 if=none,id=d1,file=${TEST_IMG},lock-mode=off' } }" \
+    "OK"
+
+_send_qemu_cmd $QEMU_HANDLE \
+    "{ 'execute': 'human-monitor-command',
+       'arguments': { 'command-line': 'drive_del d1' } }" \
+    ""
+
+_run_cmd $QEMU_IMG info "${TEST_IMG}"
+
+_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..991f84c
--- /dev/null
+++ b/tests/qemu-iotests/153.out
@@ -0,0 +1,426 @@
+QA output created by 153
+
+== 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  ==
+
+== Launching another QEMU  ==
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,if=none,: Failed to lock image
+
+== Launching another QEMU lock-mode=exclusive ==
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,if=none,lock-mode=exclusive: Failed to lock image
+
+== Launching another QEMU lock-mode=shared ==
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,if=none,lock-mode=shared: Failed to lock image
+
+== Launching another QEMU lock-mode=off ==
+
+== Launching another QEMU lock-mode=exclusive,readonly=on ==
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,if=none,lock-mode=exclusive,readonly=on: Failed to lock image
+
+== Running utility commands  ==
+
+_qemu_io_wrapper -c read 0 512 TEST_DIR/t.qcow2
+can't open device TEST_DIR/t.qcow2: Failed to lock image
+no file open, try 'help open'
+
+_qemu_io_wrapper -r -c read 0 512 TEST_DIR/t.qcow2
+can't open device TEST_DIR/t.qcow2: Failed to lock image
+no file open, try 'help open'
+
+_qemu_img_wrapper info TEST_DIR/t.qcow2
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to lock image
+
+_qemu_img_wrapper check TEST_DIR/t.qcow2
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to lock image
+
+_qemu_img_wrapper compare TEST_DIR/t.qcow2 TEST_DIR/t.qcow2
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to lock image
+
+_qemu_img_wrapper map TEST_DIR/t.qcow2
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to lock image
+
+_qemu_img_wrapper amend -o  TEST_DIR/t.qcow2
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to lock image
+
+_qemu_img_wrapper commit TEST_DIR/t.qcow2
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to lock image
+
+_qemu_img_wrapper resize TEST_DIR/t.qcow2 32M
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to lock 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 lock image
+
+_qemu_img_wrapper snapshot -l TEST_DIR/t.qcow2
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to lock 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 lock image
+
+== Running utility commands (lock disabled) ==
+
+_qemu_io_wrapper -L -c read 0 512 TEST_DIR/t.qcow2
+
+_qemu_io_wrapper -L -r -c read 0 512 TEST_DIR/t.qcow2
+
+_qemu_img_wrapper info -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper check -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper compare -L TEST_DIR/t.qcow2 TEST_DIR/t.qcow2
+
+_qemu_img_wrapper map -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper amend -o  -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper commit -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper resize -L TEST_DIR/t.qcow2 32M
+
+_qemu_img_wrapper rebase -L TEST_DIR/t.qcow2 -b TEST_DIR/t.qcow2.base
+
+_qemu_img_wrapper snapshot -l -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper convert -L TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.convert
+
+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 readonly=on ==
+
+== Launching another QEMU  ==
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,if=none,: Failed to lock image
+
+== Launching another QEMU lock-mode=exclusive ==
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,if=none,lock-mode=exclusive: Failed to lock image
+
+== Launching another QEMU lock-mode=shared ==
+
+== Launching another QEMU lock-mode=off ==
+
+== Launching another QEMU lock-mode=exclusive,readonly=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 lock image
+no file open, try 'help open'
+
+_qemu_io_wrapper -r -c read 0 512 TEST_DIR/t.qcow2
+
+_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 lock image
+
+_qemu_img_wrapper commit TEST_DIR/t.qcow2
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to lock image
+
+_qemu_img_wrapper resize TEST_DIR/t.qcow2 32M
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to lock 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 lock image
+
+_qemu_img_wrapper snapshot -l TEST_DIR/t.qcow2
+
+_qemu_img_wrapper convert TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.convert
+
+== Running utility commands (lock disabled) ==
+
+_qemu_io_wrapper -L -c read 0 512 TEST_DIR/t.qcow2
+
+_qemu_io_wrapper -L -r -c read 0 512 TEST_DIR/t.qcow2
+
+_qemu_img_wrapper info -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper check -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper compare -L TEST_DIR/t.qcow2 TEST_DIR/t.qcow2
+
+_qemu_img_wrapper map -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper amend -o  -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper commit -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper resize -L TEST_DIR/t.qcow2 32M
+
+_qemu_img_wrapper rebase -L TEST_DIR/t.qcow2 -b TEST_DIR/t.qcow2.base
+
+_qemu_img_wrapper snapshot -l -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper convert -L TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.convert
+
+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 lock-mode=off ==
+
+== Launching another QEMU  ==
+
+== Launching another QEMU lock-mode=exclusive ==
+
+== Launching another QEMU lock-mode=shared ==
+
+== Launching another QEMU lock-mode=off ==
+
+== Launching another QEMU lock-mode=exclusive,readonly=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_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
+
+== Running utility commands (lock disabled) ==
+
+_qemu_io_wrapper -L -c read 0 512 TEST_DIR/t.qcow2
+
+_qemu_io_wrapper -L -r -c read 0 512 TEST_DIR/t.qcow2
+
+_qemu_img_wrapper info -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper check -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper compare -L TEST_DIR/t.qcow2 TEST_DIR/t.qcow2
+
+_qemu_img_wrapper map -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper amend -o  -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper commit -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper resize -L TEST_DIR/t.qcow2 32M
+
+_qemu_img_wrapper rebase -L TEST_DIR/t.qcow2 -b TEST_DIR/t.qcow2.base
+
+_qemu_img_wrapper snapshot -l -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper convert -L TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.convert
+
+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 lock-mode=shared ==
+
+== Launching another QEMU  ==
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,if=none,: Failed to lock image
+
+== Launching another QEMU lock-mode=exclusive ==
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,if=none,lock-mode=exclusive: Failed to lock image
+
+== Launching another QEMU lock-mode=shared ==
+
+== Launching another QEMU lock-mode=off ==
+
+== Launching another QEMU lock-mode=exclusive,readonly=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 lock image
+no file open, try 'help open'
+
+_qemu_io_wrapper -r -c read 0 512 TEST_DIR/t.qcow2
+
+_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 lock image
+
+_qemu_img_wrapper commit TEST_DIR/t.qcow2
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to lock image
+
+_qemu_img_wrapper resize TEST_DIR/t.qcow2 32M
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to lock 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 lock image
+
+_qemu_img_wrapper snapshot -l TEST_DIR/t.qcow2
+
+_qemu_img_wrapper convert TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.convert
+
+== Running utility commands (lock disabled) ==
+
+_qemu_io_wrapper -L -c read 0 512 TEST_DIR/t.qcow2
+
+_qemu_io_wrapper -L -r -c read 0 512 TEST_DIR/t.qcow2
+
+_qemu_img_wrapper info -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper check -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper compare -L TEST_DIR/t.qcow2 TEST_DIR/t.qcow2
+
+_qemu_img_wrapper map -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper amend -o  -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper commit -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper resize -L TEST_DIR/t.qcow2 32M
+
+_qemu_img_wrapper rebase -L TEST_DIR/t.qcow2 -b TEST_DIR/t.qcow2.base
+
+_qemu_img_wrapper snapshot -l -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper convert -L TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.convert
+
+Round done
+
+== Two devices with the same image (lock-mode=shared,readonly=on - lock-mode=shared,readonly=on) ==
+
+== Two devices with the same image (lock-mode=shared,readonly=on - lock-mode=shared,readonly=off) ==
+
+== Two devices with the same image (lock-mode=shared,readonly=on - lock-mode=exclusive,readonly=on) ==
+
+== Two devices with the same image (lock-mode=shared,readonly=on - lock-mode=exclusive,readonly=off) ==
+QEMU_PROG: -drive if=none,file=TEST_DIR/t.qcow2,lock-mode=exclusive,readonly=off: Failed to lock image
+
+== Two devices with the same image (lock-mode=shared,readonly=on - lock-mode=off,readonly=on) ==
+
+== Two devices with the same image (lock-mode=shared,readonly=on - lock-mode=off,readonly=off) ==
+
+== Two devices with the same image (lock-mode=shared,readonly=off - lock-mode=shared,readonly=on) ==
+
+== Two devices with the same image (lock-mode=shared,readonly=off - lock-mode=shared,readonly=off) ==
+
+== Two devices with the same image (lock-mode=shared,readonly=off - lock-mode=exclusive,readonly=on) ==
+
+== Two devices with the same image (lock-mode=shared,readonly=off - lock-mode=exclusive,readonly=off) ==
+QEMU_PROG: -drive if=none,file=TEST_DIR/t.qcow2,lock-mode=exclusive,readonly=off: Failed to lock image
+
+== Two devices with the same image (lock-mode=shared,readonly=off - lock-mode=off,readonly=on) ==
+
+== Two devices with the same image (lock-mode=shared,readonly=off - lock-mode=off,readonly=off) ==
+
+== Two devices with the same image (lock-mode=exclusive,readonly=on - lock-mode=shared,readonly=on) ==
+
+== Two devices with the same image (lock-mode=exclusive,readonly=on - lock-mode=shared,readonly=off) ==
+
+== Two devices with the same image (lock-mode=exclusive,readonly=on - lock-mode=exclusive,readonly=on) ==
+
+== Two devices with the same image (lock-mode=exclusive,readonly=on - lock-mode=exclusive,readonly=off) ==
+QEMU_PROG: -drive if=none,file=TEST_DIR/t.qcow2,lock-mode=exclusive,readonly=off: Failed to lock image
+
+== Two devices with the same image (lock-mode=exclusive,readonly=on - lock-mode=off,readonly=on) ==
+
+== Two devices with the same image (lock-mode=exclusive,readonly=on - lock-mode=off,readonly=off) ==
+
+== Two devices with the same image (lock-mode=exclusive,readonly=off - lock-mode=shared,readonly=on) ==
+QEMU_PROG: -drive if=none,file=TEST_DIR/t.qcow2,lock-mode=shared,readonly=on: Failed to lock image
+
+== Two devices with the same image (lock-mode=exclusive,readonly=off - lock-mode=shared,readonly=off) ==
+QEMU_PROG: -drive if=none,file=TEST_DIR/t.qcow2,lock-mode=shared,readonly=off: Failed to lock image
+
+== Two devices with the same image (lock-mode=exclusive,readonly=off - lock-mode=exclusive,readonly=on) ==
+QEMU_PROG: -drive if=none,file=TEST_DIR/t.qcow2,lock-mode=exclusive,readonly=on: Failed to lock image
+
+== Two devices with the same image (lock-mode=exclusive,readonly=off - lock-mode=exclusive,readonly=off) ==
+QEMU_PROG: -drive if=none,file=TEST_DIR/t.qcow2,lock-mode=exclusive,readonly=off: Failed to lock image
+
+== Two devices with the same image (lock-mode=exclusive,readonly=off - lock-mode=off,readonly=on) ==
+
+== Two devices with the same image (lock-mode=exclusive,readonly=off - lock-mode=off,readonly=off) ==
+
+== Two devices with the same image (lock-mode=off,readonly=on - lock-mode=shared,readonly=on) ==
+
+== Two devices with the same image (lock-mode=off,readonly=on - lock-mode=shared,readonly=off) ==
+
+== Two devices with the same image (lock-mode=off,readonly=on - lock-mode=exclusive,readonly=on) ==
+
+== Two devices with the same image (lock-mode=off,readonly=on - lock-mode=exclusive,readonly=off) ==
+
+== Two devices with the same image (lock-mode=off,readonly=on - lock-mode=off,readonly=on) ==
+
+== Two devices with the same image (lock-mode=off,readonly=on - lock-mode=off,readonly=off) ==
+
+== Two devices with the same image (lock-mode=off,readonly=off - lock-mode=shared,readonly=on) ==
+
+== Two devices with the same image (lock-mode=off,readonly=off - lock-mode=shared,readonly=off) ==
+
+== Two devices with the same image (lock-mode=off,readonly=off - lock-mode=exclusive,readonly=on) ==
+
+== Two devices with the same image (lock-mode=off,readonly=off - lock-mode=exclusive,readonly=off) ==
+
+== Two devices with the same image (lock-mode=off,readonly=off - lock-mode=off,readonly=on) ==
+
+== Two devices with the same image (lock-mode=off,readonly=off - lock-mode=off,readonly=off) ==
+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 lock image
+
+== Backing image also as an active device (ro) ==
+
+== Backing image also as an active device (shared) ==
+
+== Symbolic link ==
+QEMU_PROG: -drive if=none,file=TEST_DIR/t.qcow2: Failed to lock image
+
+== Test that close one BDS doesn't unlock others ==
+{"return": {}}
+{"return": "Failed to lock imagern"}
+{"return": "OKrn"}
+
+_qemu_img_wrapper info TEST_DIR/t.qcow2
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to lock image
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 822953b..3f6d4a9 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -153,3 +153,4 @@
 149 rw auto sudo
 150 rw auto quick
 152 rw auto quick
+153 rw auto quick
-- 
2.8.2

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

[Max Reitz]

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

On 17.05.2016 09:35, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  tests/qemu-iotests/153     | 197 +++++++++++++++++++++
>  tests/qemu-iotests/153.out | 426 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/group   |   1 +
>  3 files changed, 624 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..cbeda8e
> --- /dev/null
> +++ b/tests/qemu-iotests/153

[...]

> +for opts1 in "" "readonly=on" "lock-mode=off" "lock-mode=shared"; 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

_make_test_img understands -b, so maybe you want to use it here.

Also I'm not sure why you're creating the images inside of this loop,
but it certainly isn't wrong.

But maybe you want to add a line that echos $opts1 somewhere here.

However you decide:

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

Re: [Qemu-devel] [PATCH v5 00/27] block: Lock images when opening

[Richard W.M. Jones]

On Tue, May 17, 2016 at 03:35:09PM +0800, Fam Zheng wrote:
> v5: - Change "lock-image=on/off" to "lock-mode=exclusive/shared/off".
>       Default is "lock-mode=exclusive" to exclusively lock RW images and shared
>       lock RO images; with lock-mode="shared", RW images are shared locked too;
>       lock-mode=off turns off image locking completely.
>     - Use F_OFD_SETLK fcntl so that close/dup on different fds are not a
>       problem.
>     - Update test cases.

My comments after testing this patch set:

* It's not possible to tell from the `qemu -help' output that this
  binary supports the lock-mode option.  Please add this to the -help
  output (under `-drive') so we can detect it in qemu.

* I patched libguestfs to add the `lock-image=off' flag when the drive
  is added readonly.  This permits libguestfs to read live guests.  I
  also checked that writing to live guests is now forbidden, and it
  is, which is good.  In the write-to-live-guest case libguestfs will
  now fail with:

  qemu-system-x86_64: -drive file=/var/tmp/centos-6.img,cache=writeback,id=hd0,if=none: Failed to lock image

So definitely we need this option to be reflected in the -help output.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

Re: [Qemu-devel] [PATCH v5 00/27] block: Lock images when opening

[Kevin Wolf]

Am 24.05.2016 um 13:48 hat Richard W.M. Jones geschrieben:
> On Tue, May 17, 2016 at 03:35:09PM +0800, Fam Zheng wrote:
> > v5: - Change "lock-image=on/off" to "lock-mode=exclusive/shared/off".
> >       Default is "lock-mode=exclusive" to exclusively lock RW images and shared
> >       lock RO images; with lock-mode="shared", RW images are shared locked too;
> >       lock-mode=off turns off image locking completely.
> >     - Use F_OFD_SETLK fcntl so that close/dup on different fds are not a
> >       problem.
> >     - Update test cases.
> 
> My comments after testing this patch set:
> 
> * It's not possible to tell from the `qemu -help' output that this
>   binary supports the lock-mode option.  Please add this to the -help
>   output (under `-drive') so we can detect it in qemu.
> 
> * I patched libguestfs to add the `lock-image=off' flag when the drive
>   is added readonly.  This permits libguestfs to read live guests.  I
>   also checked that writing to live guests is now forbidden, and it
>   is, which is good.  In the write-to-live-guest case libguestfs will
>   now fail with:
> 
>   qemu-system-x86_64: -drive file=/var/tmp/centos-6.img,cache=writeback,id=hd0,if=none: Failed to lock image
> 
> So definitely we need this option to be reflected in the -help output.

While you are right that maybe we should mentioned the new options in
-help for human users, help texts are not meant to be parsed. If you do
parse them and a changed help text breaks your code in the future,
that's your problem.

Consider using query-qmp-schema instead, that one is actually meant to
be processed by machines.

Kevin

Re: [Qemu-devel] [PATCH v5 00/27] block: Lock images when opening

[Richard W.M. Jones]

On Tue, May 24, 2016 at 02:46:15PM +0200, Kevin Wolf wrote:
> Am 24.05.2016 um 13:48 hat Richard W.M. Jones geschrieben:
> > On Tue, May 17, 2016 at 03:35:09PM +0800, Fam Zheng wrote:
> > > v5: - Change "lock-image=on/off" to "lock-mode=exclusive/shared/off".
> > >       Default is "lock-mode=exclusive" to exclusively lock RW images and shared
> > >       lock RO images; with lock-mode="shared", RW images are shared locked too;
> > >       lock-mode=off turns off image locking completely.
> > >     - Use F_OFD_SETLK fcntl so that close/dup on different fds are not a
> > >       problem.
> > >     - Update test cases.
> > 
> > My comments after testing this patch set:
> > 
> > * It's not possible to tell from the `qemu -help' output that this
> >   binary supports the lock-mode option.  Please add this to the -help
> >   output (under `-drive') so we can detect it in qemu.
> > 
> > * I patched libguestfs to add the `lock-image=off' flag when the drive
> >   is added readonly.  This permits libguestfs to read live guests.  I
> >   also checked that writing to live guests is now forbidden, and it
> >   is, which is good.  In the write-to-live-guest case libguestfs will
> >   now fail with:
> > 
> >   qemu-system-x86_64: -drive file=/var/tmp/centos-6.img,cache=writeback,id=hd0,if=none: Failed to lock image
> > 
> > So definitely we need this option to be reflected in the -help output.
> 
> While you are right that maybe we should mentioned the new options in
> -help for human users, help texts are not meant to be parsed. If you do
> parse them and a changed help text breaks your code in the future,
> that's your problem.
> 
> Consider using query-qmp-schema instead, that one is actually meant to
> be processed by machines.

Yeah it's probably about time to do this now.  Previously it wasn't
done because opening a monitor port is slow (adds 60ms just to open
it, plus the time taken doing round trips to get the data).  However
we are now memoizing the results of querying qemu so this is no longer
critical.  I will have a look at how difficult it will be to query
this.

Anyway doesn't change the fact we need to be able to automatically
detect if the lock-mode flag is needed, given a random qemu binary.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top