[PATCH] hw/nvme/ctrl: fix functions style

[Gollu Appalanaidu]

Identify command related functions style fix.

Signed-off-by: Gollu Appalanaidu 
---
 hw/nvme/ctrl.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 0bcaf7192f..40a7efcea9 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -4291,7 +4291,7 @@ static uint16_t nvme_identify_ns_attached_list(NvmeCtrl 
*n, NvmeRequest *req)
 }
 
 static uint16_t nvme_identify_ns_csi(NvmeCtrl *n, NvmeRequest *req,
-bool active)
+ bool active)
 {
 NvmeNamespace *ns;
 NvmeIdentify *c = (NvmeIdentify *)&req->cmd;
@@ -4326,7 +4326,7 @@ static uint16_t nvme_identify_ns_csi(NvmeCtrl *n, 
NvmeRequest *req,
 }
 
 static uint16_t nvme_identify_nslist(NvmeCtrl *n, NvmeRequest *req,
-bool active)
+ bool active)
 {
 NvmeNamespace *ns;
 NvmeIdentify *c = (NvmeIdentify *)&req->cmd;
@@ -4373,7 +4373,7 @@ static uint16_t nvme_identify_nslist(NvmeCtrl *n, 
NvmeRequest *req,
 }
 
 static uint16_t nvme_identify_nslist_csi(NvmeCtrl *n, NvmeRequest *req,
-bool active)
+ bool active)
 {
 NvmeNamespace *ns;
 NvmeIdentify *c = (NvmeIdentify *)&req->cmd;
-- 
2.17.1

Re: [PATCH v6 04/25] python: add qemu package installer

[Cleber Rosa]

On Wed, May 12, 2021 at 07:12:20PM -0400, John Snow wrote:
> Add setup.cfg and setup.py, necessary for installing a package via
> pip. Add a ReST document (PACKAGE.rst) explaining the basics of what
> this package is for and who to contact for more information. This
> document will be used as the landing page for the package on PyPI.
> 
> I am not yet using a pyproject.toml style package manifest, because
> "editable" installs are not defined (yet?) by PEP-517/518.
> 
> I consider editable installs crucial for development, though they have
> (apparently) always been somewhat poorly defined.
>

They are crucial for development indeed, so I agree with your overall
choices here.

> Pip now (19.2 and later) now supports editable installs for projects
> using pyproject.toml manifests, but might require the use of the
> --no-use-pep517 flag, which somewhat defeats the point.
>

Just to make it clear for other people reading this, it means that
even with the very latest pip release (21.1.1), you *must* have a
setup.py or setup.cfg file to use editable (development) installs[1]
You can *not* rely solely on a pyproject.toml setup.

[1] 
https://github.com/pypa/pip/pull/9547/commits/7a95720e796a5e56481c1cc20b6ce6249c50f357

> For now, while the dust settles, stick with the de-facto
> setup.py/setup.cfg combination supported by setuptools. It will be worth
> re-evaluating this point again in the future when our supported build
> platforms all ship a fairly modern pip.
>

Agreed, but let's not hold our breath given that even pip 21.1.1 still
doesn't support that.  My guesstimate is 6-12 months for the feature to
be developed/merged, and let's say another 12 months for our supported
build platforms to ship it.

> Additional reading on this matter:
> 
> https://github.com/pypa/packaging-problems/issues/256
> https://github.com/pypa/pip/issues/6334
> https://github.com/pypa/pip/issues/6375
> https://github.com/pypa/pip/issues/6434
> https://github.com/pypa/pip/issues/6438
> 
> Signed-off-by: John Snow 
> ---
>  python/PACKAGE.rst | 33 +
>  python/setup.cfg   | 19 +++
>  python/setup.py| 23 +++
>  3 files changed, 75 insertions(+)
>  create mode 100644 python/PACKAGE.rst
>  create mode 100644 python/setup.cfg
>  create mode 100755 python/setup.py
>
> diff --git a/python/PACKAGE.rst b/python/PACKAGE.rst
> new file mode 100644
> index 000..1bbfe1b58e2
> --- /dev/null
> +++ b/python/PACKAGE.rst
> @@ -0,0 +1,33 @@
> +QEMU Python Tooling
> +===
> +
> +This package provides QEMU tooling used by the QEMU project to build,
> +configure, and test QEMU. It is not a fully-fledged SDK and it is subject
> +to change at any time.
> +
> +Usage
> +-
> +
> +The ``qemu.qmp`` subpackage provides a library for communicating with
> +QMP servers. The ``qemu.machine`` subpackage offers rudimentary
> +facilities for launching and managing QEMU processes. Refer to each
> +package's documentation
> +(``>>> help(qemu.qmp)``, ``>>> help(qemu.machine)``)
> +for more information.
> +
> +Contributing
> +
> +
> +This package is maintained by John Snow  as part of
> +the QEMU source tree. Contributions are welcome and follow the `QEMU
> +patch submission process
> +`_, which involves
> +sending patches to the QEMU development mailing list.
> +
> +John maintains a `GitLab staging branch
> +`_, and there is an
> +official `GitLab mirror `_.
> +
> +Please report bugs on the `QEMU issue tracker
> +`_ and tag ``@jsnow`` in
> +the report.
> diff --git a/python/setup.cfg b/python/setup.cfg
> new file mode 100644
> index 000..dd71640fc2f
> --- /dev/null
> +++ b/python/setup.cfg
> @@ -0,0 +1,19 @@
> +[metadata]
> +name = qemu
> +maintainer = QEMU Developer Team
> +maintainer_email = qemu-de...@nongnu.org
> +url = https://www.qemu.org/
> +download_url = https://www.qemu.org/download/
> +description = QEMU Python Build, Debug and SDK tooling.
> +long_description = file:PACKAGE.rst
> +long_description_content_type = text/x-rst
> +classifiers =
> +Development Status :: 3 - Alpha
> +License :: OSI Approved :: GNU General Public License v2 (GPLv2)
> +Natural Language :: English
> +Operating System :: OS Independent
> +Programming Language :: Python :: 3 :: Only
> +
> +[options]
> +python_requires = >= 3.6
> +packages = find_namespace:
> diff --git a/python/setup.py b/python/setup.py
> new file mode 100755
> index 000..2014f81b757
> --- /dev/null
> +++ b/python/setup.py
> @@ -0,0 +1,23 @@
> +#!/usr/bin/env python3
> +"""
> +QEMU tooling installer script
> +Copyright (c) 2020-2021 John Snow for Red Hat, Inc.
> +"""
> +
> +import setuptools
> +import pkg_resources
> +
> +
> +def main():
> +"""
> +QEMU tooling installer
> +"""
> +
> +# 
>

Re: [PULL 0/9] scripts/simplebench patches

[Vladimir Sementsov-Ogievskiy]

20.05.2021 22:17, Peter Maydell wrote:

On Tue, 4 May 2021 at 10:01, Vladimir Sementsov-Ogievskiy
 wrote:


The following changes since commit 53c5433e84e8935abed8e91d4a2eb813168a0ecf:

   Merge remote-tracking branch 'remotes/rth-gitlab/tags/pull-tcg-20210501' 
into staging (2021-05-02 12:02:46 +0100)

are available in the Git repository at:

   https://src.openvz.org/scm/~vsementsov/qemu.git 
tags/pull-simplebench-2021-05-04

for you to fetch changes up to e34bd02694026722410b80cee02ab7f33f893e9b:

   MAINTAINERS: update Benchmark util: add git tree (2021-05-04 11:37:26 +0300)


scripts/simplebench improvements for 2021-05-04



I couldn't find the gpg key you signed this with on the public
keyserver. Could you point me at where you uploaded it, please?



Here it is: 
http://keys.gnupg.net/pks/lookup?op=vindex&fingerprint=on&search=0x561F24C1F19F79FB


--
Best regards,
Vladimir

Re: [PULL 0/9] scripts/simplebench patches

[Peter Maydell]

On Tue, 4 May 2021 at 10:01, Vladimir Sementsov-Ogievskiy
 wrote:
>
> The following changes since commit 53c5433e84e8935abed8e91d4a2eb813168a0ecf:
>
>   Merge remote-tracking branch 'remotes/rth-gitlab/tags/pull-tcg-20210501' 
> into staging (2021-05-02 12:02:46 +0100)
>
> are available in the Git repository at:
>
>   https://src.openvz.org/scm/~vsementsov/qemu.git 
> tags/pull-simplebench-2021-05-04
>
> for you to fetch changes up to e34bd02694026722410b80cee02ab7f33f893e9b:
>
>   MAINTAINERS: update Benchmark util: add git tree (2021-05-04 11:37:26 +0300)
>
> 
> scripts/simplebench improvements for 2021-05-04
>

I couldn't find the gpg key you signed this with on the public
keyserver. Could you point me at where you uploaded it, please?

thanks
-- PMM

Re: [PATCH v2 6/7] block-copy: atomic .cancelled and .finished fields in BlockCopyCallState

[Vladimir Sementsov-Ogievskiy]

18.05.2021 13:07, Emanuele Giuseppe Esposito wrote:

By adding acquire/release pairs, we ensure that .ret and .error_is_read
fields are written by block_copy_dirty_clusters before .finished is true.


As I already said, please, can we live with one mutex for now? finished, ret, 
error_is_read, all these variables are changing rarely. I doubt that 
performance is improved by these atomic operations. But complexity of the 
architecture increases exponentially.



Signed-off-by: Emanuele Giuseppe Esposito 
---
  block/block-copy.c | 33 ++---
  1 file changed, 18 insertions(+), 15 deletions(-)

diff --git a/block/block-copy.c b/block/block-copy.c
index d5ed5932b0..573e96fefb 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -55,11 +55,11 @@ typedef struct BlockCopyCallState {
  QLIST_ENTRY(BlockCopyCallState) list;
  
  /* State */

-bool finished;
+bool finished; /* atomic */
  QemuCoSleep sleep; /* TODO: protect API with a lock */
  
  /* OUT parameters */

-bool cancelled;
+bool cancelled; /* atomic */
  /* Fields protected by calls_lock in BlockCopyState */
  bool error_is_read;
  int ret;
@@ -646,7 +646,8 @@ block_copy_dirty_clusters(BlockCopyCallState *call_state)
  assert(QEMU_IS_ALIGNED(offset, s->cluster_size));
  assert(QEMU_IS_ALIGNED(bytes, s->cluster_size));
  
-while (bytes && aio_task_pool_status(aio) == 0 && !call_state->cancelled) {

+while (bytes && aio_task_pool_status(aio) == 0 &&
+   !qatomic_read(&call_state->cancelled)) {
  BlockCopyTask *task;
  int64_t status_bytes;
  
@@ -754,7 +755,7 @@ static int coroutine_fn block_copy_common(BlockCopyCallState *call_state)

  do {
  ret = block_copy_dirty_clusters(call_state);
  
-if (ret == 0 && !call_state->cancelled) {

+if (ret == 0 && !qatomic_read(&call_state->cancelled)) {
  ret = block_copy_wait_one(call_state->s, call_state->offset,
call_state->bytes);
  }
@@ -768,9 +769,9 @@ static int coroutine_fn 
block_copy_common(BlockCopyCallState *call_state)
   * 2. We have waited for some intersecting block-copy request
   *It may have failed and produced new dirty bits.
   */
-} while (ret > 0 && !call_state->cancelled);
+} while (ret > 0 && !qatomic_read(&call_state->cancelled));
  
-call_state->finished = true;

+qatomic_store_release(&call_state->finished, true);
  
  if (call_state->cb) {

  call_state->cb(call_state->cb_opaque);
@@ -833,35 +834,37 @@ void block_copy_call_free(BlockCopyCallState *call_state)
  return;
  }
  
-assert(call_state->finished);

+assert(qatomic_load_acquire(&call_state->finished));
  g_free(call_state);
  }
  
  bool block_copy_call_finished(BlockCopyCallState *call_state)

  {
-return call_state->finished;
+return qatomic_load_acquire(&call_state->finished);
  }
  
  bool block_copy_call_succeeded(BlockCopyCallState *call_state)

  {
-return call_state->finished && !call_state->cancelled &&
-call_state->ret == 0;
+return qatomic_load_acquire(&call_state->finished) &&
+   !qatomic_read(&call_state->cancelled) &&
+   call_state->ret == 0;
  }
  
  bool block_copy_call_failed(BlockCopyCallState *call_state)

  {
-return call_state->finished && !call_state->cancelled &&
-call_state->ret < 0;
+return qatomic_load_acquire(&call_state->finished) &&
+   !qatomic_read(&call_state->cancelled) &&
+   call_state->ret < 0;
  }
  
  bool block_copy_call_cancelled(BlockCopyCallState *call_state)

  {
-return call_state->cancelled;
+return qatomic_read(&call_state->cancelled);
  }
  
  int block_copy_call_status(BlockCopyCallState *call_state, bool *error_is_read)

  {
-assert(call_state->finished);
+assert(qatomic_load_acquire(&call_state->finished));
  if (error_is_read) {
  *error_is_read = call_state->error_is_read;
  }
@@ -870,7 +873,7 @@ int block_copy_call_status(BlockCopyCallState *call_state, 
bool *error_is_read)
  
  void block_copy_call_cancel(BlockCopyCallState *call_state)

  {
-call_state->cancelled = true;
+qatomic_set(&call_state->cancelled, true);
  block_copy_kick(call_state);
  }
  




--
Best regards,
Vladimir

Re: [PATCH v2 5/7] block-copy: add QemuMutex lock for BlockCopyCallState list

[Vladimir Sementsov-Ogievskiy]

18.05.2021 13:07, Emanuele Giuseppe Esposito wrote:

As for BlockCopyTask, add a lock to protect BlockCopyCallState
ret and sleep_state fields. Also move ret, finished and cancelled
in the OUT fields of BlockCopyCallState.

Here a QemuMutex is used to protect QemuCoSleep field, since it
can be concurrently invoked also from outside threads.

.finished, .cancelled and reads to .ret and .error_is_read will be
protected in the following patch.

.sleep state is handled in the series "coroutine: new sleep/wake API"


Could we live with one mutex for all needs? Why to add one more?



Signed-off-by: Emanuele Giuseppe Esposito 
---
  block/block-copy.c | 27 +++
  1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/block/block-copy.c b/block/block-copy.c
index 3a949fab64..d5ed5932b0 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -55,13 +55,14 @@ typedef struct BlockCopyCallState {
  QLIST_ENTRY(BlockCopyCallState) list;
  
  /* State */

-int ret;
  bool finished;
-QemuCoSleep sleep;
-bool cancelled;
+QemuCoSleep sleep; /* TODO: protect API with a lock */
  
  /* OUT parameters */

+bool cancelled;
+/* Fields protected by calls_lock in BlockCopyState */
  bool error_is_read;
+int ret;
  } BlockCopyCallState;
  
  typedef struct BlockCopyTask {

@@ -110,6 +111,7 @@ typedef struct BlockCopyState {
  BlockCopyMethod method;
  CoMutex tasks_lock;
  QLIST_HEAD(, BlockCopyTask) tasks; /* All tasks from all block-copy calls 
*/
+QemuMutex calls_lock;
  QLIST_HEAD(, BlockCopyCallState) calls;
  /* State fields that use a thread-safe API */
  BdrvDirtyBitmap *copy_bitmap;
@@ -289,6 +291,7 @@ void block_copy_state_free(BlockCopyState *s)
  }
  
  ratelimit_destroy(&s->rate_limit);

+qemu_mutex_destroy(&s->calls_lock);
  bdrv_release_dirty_bitmap(s->copy_bitmap);
  shres_destroy(s->mem);
  g_free(s);
@@ -349,6 +352,7 @@ BlockCopyState *block_copy_state_new(BdrvChild *source, 
BdrvChild *target,
  
  ratelimit_init(&s->rate_limit);

  qemu_co_mutex_init(&s->tasks_lock);
+qemu_mutex_init(&s->calls_lock);
  QLIST_INIT(&s->tasks);
  QLIST_INIT(&s->calls);
  
@@ -492,11 +496,14 @@ static coroutine_fn int block_copy_task_entry(AioTask *task)
  
  ret = block_copy_do_copy(t->s, t->offset, t->bytes, t->zeroes,

   &error_is_read);
-if (ret < 0 && !t->call_state->ret) {
-t->call_state->ret = ret;
-t->call_state->error_is_read = error_is_read;
-} else {
-progress_work_done(t->s->progress, t->bytes);
+
+WITH_QEMU_LOCK_GUARD(&t->s->calls_lock) {
+if (ret < 0 && !t->call_state->ret) {
+t->call_state->ret = ret;
+t->call_state->error_is_read = error_is_read;
+} else {
+progress_work_done(t->s->progress, t->bytes);
+}
  }
  co_put_to_shres(t->s->mem, t->bytes);
  block_copy_task_end(t, ret);
@@ -740,7 +747,9 @@ static int coroutine_fn 
block_copy_common(BlockCopyCallState *call_state)
  {
  int ret;
  
+qemu_mutex_lock(&call_state->s->calls_lock);

  QLIST_INSERT_HEAD(&call_state->s->calls, call_state, list);
+qemu_mutex_unlock(&call_state->s->calls_lock);
  
  do {

  ret = block_copy_dirty_clusters(call_state);
@@ -767,7 +776,9 @@ static int coroutine_fn 
block_copy_common(BlockCopyCallState *call_state)
  call_state->cb(call_state->cb_opaque);
  }
  
+qemu_mutex_lock(&call_state->s->calls_lock);

  QLIST_REMOVE(call_state, list);
+qemu_mutex_unlock(&call_state->s->calls_lock);
  
  return ret;

  }




--
Best regards,
Vladimir

Re: [PATCH v2 1/7] block-copy: streamline choice of copy_range vs. read/write

[Vladimir Sementsov-Ogievskiy]

20.05.2021 18:06, Emanuele Giuseppe Esposito wrote:



On 20/05/2021 16:42, Vladimir Sementsov-Ogievskiy wrote:

18.05.2021 13:07, Emanuele Giuseppe Esposito wrote:

From: Paolo Bonzini 

Put the logic to determine the copy size in a separate function, so
that there is a simple state machine for the possible methods of
copying data from one BlockDriverState to the other.


Honestly, for me 4-state state-maching + function to determine copy-size 
doesn't seem better than two simple variables copy_size and use_copy_range.

What's the benefit of it?


It helps having a single field (method) instead of two (use_copy_range, 
copy_size), especially if we need to take care of protecting it for concurrent 
access. See patch 7.


What's the problem with protecting two fields?

(me looking at patch 7) What's the reason of introducing atomic operations? It 
makes things more complicated (we have two synchronization mechanisms (mutex + 
atomics) instead of one (mutext)), with no benefit.





While at it, store the common computation of block_copy_max_transfer
into a new field of BlockCopyState, and make sure that we always
obey max_transfer; that's more efficient even for the
COPY_RANGE_READ_WRITE case.


hmm, maybe. It could be a separate patch.



Signed-off-by: Emanuele Giuseppe Esposito 
Signed-off-by: Paolo Bonzini 
---
  block/block-copy.c | 59 ++


stats agree with me, that its' not a simplification.


  1 file changed, 39 insertions(+), 20 deletions(-)

diff --git a/block/block-copy.c b/block/block-copy.c
index 37c8e8504b..10ce51a244 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -28,6 +28,13 @@
  #define BLOCK_COPY_MAX_WORKERS 64
  #define BLOCK_COPY_SLICE_TIME 1ULL /* ns */
+typedef enum {
+    COPY_READ_WRITE_CLUSTER,
+    COPY_READ_WRITE,
+    COPY_RANGE_SMALL,
+    COPY_RANGE_FULL
+} BlockCopyMethod;
+
  static coroutine_fn int block_copy_task_entry(AioTask *task);
  typedef struct BlockCopyCallState {
@@ -85,8 +92,8 @@ typedef struct BlockCopyState {
  BdrvDirtyBitmap *copy_bitmap;
  int64_t in_flight_bytes;
  int64_t cluster_size;
-    bool use_copy_range;
-    int64_t copy_size;
+    BlockCopyMethod method;
+    int64_t max_transfer;
  uint64_t len;
  QLIST_HEAD(, BlockCopyTask) tasks; /* All tasks from all block-copy calls 
*/
  QLIST_HEAD(, BlockCopyCallState) calls;
@@ -148,6 +155,23 @@ static bool coroutine_fn 
block_copy_wait_one(BlockCopyState *s, int64_t offset,
  return true;
  }
+static inline int64_t block_copy_chunk_size(BlockCopyState *s)


"inline" word does nothing in static definitions in c files. Compiler make a 
decision independently of it.


Typo



+{
+    switch (s->method) {
+    case COPY_READ_WRITE_CLUSTER:
+    return s->cluster_size;
+    case COPY_READ_WRITE:
+    case COPY_RANGE_SMALL:
+    return MIN(MAX(s->cluster_size, BLOCK_COPY_MAX_BUFFER),
+   s->max_transfer);
+    case COPY_RANGE_FULL:
+    return MIN(MAX(s->cluster_size, BLOCK_COPY_MAX_COPY_RANGE),
+   s->max_transfer);
+    default:
+    abort();
+    }
+}
+
  /*
   * Search for the first dirty area in offset/bytes range and create task at
   * the beginning of it.
@@ -157,8 +181,9 @@ static BlockCopyTask *block_copy_task_create(BlockCopyState 
*s,
   int64_t offset, int64_t bytes)
  {
  BlockCopyTask *task;
-    int64_t max_chunk = MIN_NON_ZERO(s->copy_size, call_state->max_chunk);
+    int64_t max_chunk = block_copy_chunk_size(s);
+    max_chunk = MIN_NON_ZERO(max_chunk, call_state->max_chunk);
  if (!bdrv_dirty_bitmap_next_dirty_area(s->copy_bitmap,
 offset, offset + bytes,
 max_chunk, &offset, &bytes))
@@ -265,28 +290,27 @@ BlockCopyState *block_copy_state_new(BdrvChild *source, 
BdrvChild *target,
  .len = bdrv_dirty_bitmap_size(copy_bitmap),
  .write_flags = write_flags,
  .mem = shres_create(BLOCK_COPY_MAX_MEM),
+    .max_transfer = QEMU_ALIGN_DOWN(block_copy_max_transfer(source, target)
+    , cluster_size),
  };
-    if (block_copy_max_transfer(source, target) < cluster_size) {
+    if (s->max_transfer < cluster_size) {
  /*
   * copy_range does not respect max_transfer. We don't want to bother
   * with requests smaller than block-copy cluster size, so fallback to
   * buffered copying (read and write respect max_transfer on their
   * behalf).
   */
-    s->use_copy_range = false;
-    s->copy_size = cluster_size;
+    s->method = COPY_READ_WRITE_CLUSTER;
  } else if (write_flags & BDRV_REQ_WRITE_COMPRESSED) {
  /* Compression supports only cluster-size writes and no copy-range. */
-    s->use_copy_range = false;
-    s->copy_size = cluster_size;
+    s->method = COPY_READ_WRIT

Re: [PATCH v2 2/7] block-copy: improve documentation of BlockCopyTask and BlockCopyState types and functions

[Emanuele Giuseppe Esposito]

On 20/05/2021 17:00, Vladimir Sementsov-Ogievskiy wrote:

18.05.2021 13:07, Emanuele Giuseppe Esposito wrote:

As done in BlockCopyCallState, categorize BlockCopyTask
and BlockCopyState in IN, State and OUT fields.
This is just to understand which field has to be protected with a lock.

BlockCopyTask .zeroes is a special case, because it is only initialized
and then read by the coroutine in block_copy_task_entry.

Also set block_copy_task_create as coroutine_fn because:
1) it is static and only invoked by coroutine functions
2) next patches will introduce and use a CoMutex lock there


this change is unrelated, why not to put it into commit, which adds use 
of CoMutex in that function?


Ok I will move it in patch 4.





Signed-off-by: Emanuele Giuseppe Esposito 


[...]


you add an empty line before group, it looks good


+    /* State */
  int64_t in_flight_bytes;
-    int64_t cluster_size;
  BlockCopyMethod method;
-    int64_t max_transfer;
-    uint64_t len;
  QLIST_HEAD(, BlockCopyTask) tasks; /* All tasks from all 
block-copy calls */

  QLIST_HEAD(, BlockCopyCallState) calls;


but not here..


Because these are still State fields, so in the same State group. It is 
a different sub-category.





+    /* State fields that use a thread-safe API */
+    BdrvDirtyBitmap *copy_bitmap;
+    ProgressMeter *progress;
+    SharedResource *mem;
+    RateLimit rate_limit;
+    /*
+ * IN parameters. Initialized in block_copy_state_new()
+ * and never changed.
+ */
+    int64_t cluster_size;
+    int64_t max_transfer;
+    uint64_t len;
  BdrvRequestFlags write_flags;
  /*


Thank you,
Emanuele

Re: [PATCH v2 1/7] block-copy: streamline choice of copy_range vs. read/write

[Emanuele Giuseppe Esposito]

On 20/05/2021 16:42, Vladimir Sementsov-Ogievskiy wrote:

18.05.2021 13:07, Emanuele Giuseppe Esposito wrote:

From: Paolo Bonzini 

Put the logic to determine the copy size in a separate function, so
that there is a simple state machine for the possible methods of
copying data from one BlockDriverState to the other.


Honestly, for me 4-state state-maching + function to determine copy-size 
doesn't seem better than two simple variables copy_size and use_copy_range.


What's the benefit of it?


It helps having a single field (method) instead of two (use_copy_range, 
copy_size), especially if we need to take care of protecting it for 
concurrent access. See patch 7.




While at it, store the common computation of block_copy_max_transfer
into a new field of BlockCopyState, and make sure that we always
obey max_transfer; that's more efficient even for the
COPY_RANGE_READ_WRITE case.


hmm, maybe. It could be a separate patch.



Signed-off-by: Emanuele Giuseppe Esposito 
Signed-off-by: Paolo Bonzini 
---
  block/block-copy.c | 59 ++


stats agree with me, that its' not a simplification.


  1 file changed, 39 insertions(+), 20 deletions(-)

diff --git a/block/block-copy.c b/block/block-copy.c
index 37c8e8504b..10ce51a244 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -28,6 +28,13 @@
  #define BLOCK_COPY_MAX_WORKERS 64
  #define BLOCK_COPY_SLICE_TIME 1ULL /* ns */
+typedef enum {
+    COPY_READ_WRITE_CLUSTER,
+    COPY_READ_WRITE,
+    COPY_RANGE_SMALL,
+    COPY_RANGE_FULL
+} BlockCopyMethod;
+
  static coroutine_fn int block_copy_task_entry(AioTask *task);
  typedef struct BlockCopyCallState {
@@ -85,8 +92,8 @@ typedef struct BlockCopyState {
  BdrvDirtyBitmap *copy_bitmap;
  int64_t in_flight_bytes;
  int64_t cluster_size;
-    bool use_copy_range;
-    int64_t copy_size;
+    BlockCopyMethod method;
+    int64_t max_transfer;
  uint64_t len;
  QLIST_HEAD(, BlockCopyTask) tasks; /* All tasks from all 
block-copy calls */

  QLIST_HEAD(, BlockCopyCallState) calls;
@@ -148,6 +155,23 @@ static bool coroutine_fn 
block_copy_wait_one(BlockCopyState *s, int64_t offset,

  return true;
  }
+static inline int64_t block_copy_chunk_size(BlockCopyState *s)


"inline" word does nothing in static definitions in c files. Compiler 
make a decision independently of it.


Typo



+{
+    switch (s->method) {
+    case COPY_READ_WRITE_CLUSTER:
+    return s->cluster_size;
+    case COPY_READ_WRITE:
+    case COPY_RANGE_SMALL:
+    return MIN(MAX(s->cluster_size, BLOCK_COPY_MAX_BUFFER),
+   s->max_transfer);
+    case COPY_RANGE_FULL:
+    return MIN(MAX(s->cluster_size, BLOCK_COPY_MAX_COPY_RANGE),
+   s->max_transfer);
+    default:
+    abort();
+    }
+}
+
  /*
   * Search for the first dirty area in offset/bytes range and create 
task at

   * the beginning of it.
@@ -157,8 +181,9 @@ static BlockCopyTask 
*block_copy_task_create(BlockCopyState *s,
   int64_t offset, int64_t 
bytes)

  {
  BlockCopyTask *task;
-    int64_t max_chunk = MIN_NON_ZERO(s->copy_size, 
call_state->max_chunk);

+    int64_t max_chunk = block_copy_chunk_size(s);
+    max_chunk = MIN_NON_ZERO(max_chunk, call_state->max_chunk);
  if (!bdrv_dirty_bitmap_next_dirty_area(s->copy_bitmap,
 offset, offset + bytes,
 max_chunk, &offset, &bytes))
@@ -265,28 +290,27 @@ BlockCopyState *block_copy_state_new(BdrvChild 
*source, BdrvChild *target,

  .len = bdrv_dirty_bitmap_size(copy_bitmap),
  .write_flags = write_flags,
  .mem = shres_create(BLOCK_COPY_MAX_MEM),
+    .max_transfer = 
QEMU_ALIGN_DOWN(block_copy_max_transfer(source, target)

+    , cluster_size),
  };
-    if (block_copy_max_transfer(source, target) < cluster_size) {
+    if (s->max_transfer < cluster_size) {
  /*
   * copy_range does not respect max_transfer. We don't want 
to bother
   * with requests smaller than block-copy cluster size, so 
fallback to
   * buffered copying (read and write respect max_transfer on 
their

   * behalf).
   */
-    s->use_copy_range = false;
-    s->copy_size = cluster_size;
+    s->method = COPY_READ_WRITE_CLUSTER;
  } else if (write_flags & BDRV_REQ_WRITE_COMPRESSED) {
  /* Compression supports only cluster-size writes and no 
copy-range. */

-    s->use_copy_range = false;
-    s->copy_size = cluster_size;
+    s->method = COPY_READ_WRITE_CLUSTER;
  } else {
  /*
   * We enable copy-range, but keep small copy_size, until first
   * successful copy_range (look at block_copy_do_copy).
   */
-    s->use_copy_range = use_copy_range;
-    s->copy_size = MAX(s->cluster_size, BLOCK_COPY_MAX_B

Re: [PATCH v2 4/7] block-copy: add a CoMutex to the BlockCopyTask list

[Vladimir Sementsov-Ogievskiy]

18.05.2021 13:07, Emanuele Giuseppe Esposito wrote:

Because the list of tasks is only modified by coroutine
functions, add a CoMutex in order to protect them.

Use the same mutex to protect also BlockCopyState in_flight_bytes
field to avoid adding additional syncronization primitives.

Signed-off-by: Emanuele Giuseppe Esposito 
---
  block/block-copy.c | 55 +-
  1 file changed, 35 insertions(+), 20 deletions(-)

diff --git a/block/block-copy.c b/block/block-copy.c
index 2e610b4142..3a949fab64 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -83,7 +83,7 @@ typedef struct BlockCopyTask {
   */
  bool zeroes;
  
-/* State */

+/* State. Protected by tasks_lock */
  CoQueue wait_queue; /* coroutines blocked on this task */
  
  /* To reference all call states from BlockCopyState */

@@ -106,8 +106,9 @@ typedef struct BlockCopyState {
  BdrvChild *target;
  
  /* State */

-int64_t in_flight_bytes;
+int64_t in_flight_bytes; /* protected by tasks_lock */


only this field is protected? or the whole State section?


  BlockCopyMethod method;
+CoMutex tasks_lock;
  QLIST_HEAD(, BlockCopyTask) tasks; /* All tasks from all block-copy calls 
*/
  QLIST_HEAD(, BlockCopyCallState) calls;
  /* State fields that use a thread-safe API */
@@ -142,8 +143,10 @@ typedef struct BlockCopyState {
  bool skip_unallocated;
  } BlockCopyState;
  
-static BlockCopyTask *find_conflicting_task(BlockCopyState *s,

-int64_t offset, int64_t bytes)
+/* Called with lock held */
+static BlockCopyTask *find_conflicting_task_locked(BlockCopyState *s,
+   int64_t offset,
+   int64_t bytes)
  {
  BlockCopyTask *t;
  
@@ -163,13 +166,16 @@ static BlockCopyTask *find_conflicting_task(BlockCopyState *s,

  static bool coroutine_fn block_copy_wait_one(BlockCopyState *s, int64_t 
offset,
   int64_t bytes)
  {
-BlockCopyTask *task = find_conflicting_task(s, offset, bytes);
+BlockCopyTask *task;
+
+QEMU_LOCK_GUARD(&s->tasks_lock);
+task = find_conflicting_task_locked(s, offset, bytes);
  
  if (!task) {

  return false;
  }
  
-qemu_co_queue_wait(&task->wait_queue, NULL);

+qemu_co_queue_wait(&task->wait_queue, &s->tasks_lock);
  
  return true;

  }
@@ -213,11 +219,7 @@ static coroutine_fn BlockCopyTask 
*block_copy_task_create(BlockCopyState *s,
  assert(QEMU_IS_ALIGNED(offset, s->cluster_size));
  bytes = QEMU_ALIGN_UP(bytes, s->cluster_size);
  


Looking at block_copy_task_create():

First, !bdrv_dirty_bitmap_next_dirty_area() doesn't take bitmaps lock, so it's 
not protected at all.

Next, even if we take bitmaps lock in bdrv_dirty_bitmap_next_dirty_area() or 
around it, it doesn't bring thread-safety to block_copy_task_create():

Imagine block_copy_task_create() is called from two threads simultaneously. Both calls 
will get same dirty area and create equal tasks. Then, 
"assert(!find_conflicting_task_locked(s, offset, bytes));" will crash.



-/* region is dirty, so no existent tasks possible in it */
-assert(!find_conflicting_task(s, offset, bytes));
-
  bdrv_reset_dirty_bitmap(s->copy_bitmap, offset, bytes);
-s->in_flight_bytes += bytes;
  
  task = g_new(BlockCopyTask, 1);

  *task = (BlockCopyTask) {
@@ -228,7 +230,13 @@ static coroutine_fn BlockCopyTask 
*block_copy_task_create(BlockCopyState *s,
  .bytes = bytes,
  };
  qemu_co_queue_init(&task->wait_queue);
-QLIST_INSERT_HEAD(&s->tasks, task, list);
+
+WITH_QEMU_LOCK_GUARD(&s->tasks_lock) {
+s->in_flight_bytes += bytes;
+/* region is dirty, so no existent tasks possible in it */
+assert(!find_conflicting_task_locked(s, offset, bytes));
+QLIST_INSERT_HEAD(&s->tasks, task, list);
+}
  
  return task;

  }
@@ -249,25 +257,29 @@ static void coroutine_fn 
block_copy_task_shrink(BlockCopyTask *task,
  
  assert(new_bytes > 0 && new_bytes < task->bytes);
  
-task->s->in_flight_bytes -= task->bytes - new_bytes;

  bdrv_set_dirty_bitmap(task->s->copy_bitmap,
task->offset + new_bytes, task->bytes - new_bytes);


Then look here. Imagine, immediately after bdrv_set_dirty_bitmap() in parallel 
thread the new task is created, which consumes these new dirty bits. Again, it 
will find conflicting task (this one, as task->bytes is not modified yet) and 
crash.

  
-task->bytes = new_bytes;

-qemu_co_queue_restart_all(&task->wait_queue);
+WITH_QEMU_LOCK_GUARD(&task->s->tasks_lock) {
+task->s->in_flight_bytes -= task->bytes - new_bytes;
+task->bytes = new_bytes;
+qemu_co_queue_restart_all(&task->wait_queue);
+}
  }
  
  static void coroutine_fn block_copy_task_end(BlockCopyTask *task, i

[PATCH 1/1] virtio: disable partitions scanning for no partitions block

[Yury Kamenev]

Signed-off-by: Yury Kamenev 
---
 hw/block/virtio-blk.c   | 4 
 include/hw/virtio/virtio-blk.h  | 1 +
 include/standard-headers/linux/virtio_blk.h | 1 +
 3 files changed, 6 insertions(+)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index f139cd7cc9..abf375b490 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -1031,6 +1031,9 @@ static uint64_t virtio_blk_get_features(VirtIODevice 
*vdev, uint64_t features,
 if (s->conf.num_queues > 1) {
 virtio_add_feature(&features, VIRTIO_BLK_F_MQ);
 }
+if (!s->conf.scan_parts) {
+virtio_add_feature(&features, VIRTIO_BLK_F_NO_PS);
+}
 
 return features;
 }
@@ -1295,6 +1298,7 @@ static Property virtio_blk_properties[] = {
 #endif
 DEFINE_PROP_BIT("request-merging", VirtIOBlock, conf.request_merging, 0,
 true),
+DEFINE_PROP_BOOL("scan-parts", VirtIOBlock, conf.scan_parts, true),
 DEFINE_PROP_UINT16("num-queues", VirtIOBlock, conf.num_queues,
VIRTIO_BLK_AUTO_NUM_QUEUES),
 DEFINE_PROP_UINT16("queue-size", VirtIOBlock, conf.queue_size, 256),
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index 29655a406d..706a789ad5 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -41,6 +41,7 @@ struct VirtIOBlkConf
 uint16_t num_queues;
 uint16_t queue_size;
 bool seg_max_adjust;
+bool scan_parts;
 bool report_discard_granularity;
 uint32_t max_discard_sectors;
 uint32_t max_write_zeroes_sectors;
diff --git a/include/standard-headers/linux/virtio_blk.h 
b/include/standard-headers/linux/virtio_blk.h
index 2dcc90826a..6b7b2db29c 100644
--- a/include/standard-headers/linux/virtio_blk.h
+++ b/include/standard-headers/linux/virtio_blk.h
@@ -40,6 +40,7 @@
 #define VIRTIO_BLK_F_MQ12  /* support more than one vq */
 #define VIRTIO_BLK_F_DISCARD   13  /* DISCARD is supported */
 #define VIRTIO_BLK_F_WRITE_ZEROES  14  /* WRITE ZEROES is supported */
+#define VIRTIO_BLK_F_NO_PS  16  /* Disable partitions scanning */
 
 /* Legacy feature bits */
 #ifndef VIRTIO_BLK_NO_LEGACY
-- 
2.24.3 (Apple Git-128)

Re: [PATCH v2 2/7] block-copy: improve documentation of BlockCopyTask and BlockCopyState types and functions

[Vladimir Sementsov-Ogievskiy]

18.05.2021 13:07, Emanuele Giuseppe Esposito wrote:

As done in BlockCopyCallState, categorize BlockCopyTask
and BlockCopyState in IN, State and OUT fields.
This is just to understand which field has to be protected with a lock.

BlockCopyTask .zeroes is a special case, because it is only initialized
and then read by the coroutine in block_copy_task_entry.

Also set block_copy_task_create as coroutine_fn because:
1) it is static and only invoked by coroutine functions
2) next patches will introduce and use a CoMutex lock there


this change is unrelated, why not to put it into commit, which adds use of 
CoMutex in that function?



Signed-off-by: Emanuele Giuseppe Esposito 
---
  block/block-copy.c | 49 --
  1 file changed, 34 insertions(+), 15 deletions(-)

diff --git a/block/block-copy.c b/block/block-copy.c
index 10ce51a244..d2d3839dec 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -67,13 +67,28 @@ typedef struct BlockCopyCallState {
  typedef struct BlockCopyTask {
  AioTask task;
  
+/*

+ * IN parameters. Initialized in block_copy_task_create()
+ * and never changed.
+ */
  BlockCopyState *s;
  BlockCopyCallState *call_state;
  int64_t offset;
-int64_t bytes;
+int64_t bytes; /* only re-set in task_shrink, before running the task */
+
+/*
+ * "local" parameter: used only to communicate between
+ * the caller (block_copy_dirty_clusters) and the aiotask
+ * coroutine running block_copy_task_entry
+ */


I a bit don't follow. bytes and offset are used for the same thing.. and bytes 
modified the same way, before running task, as you write in a comment. Why 
zeroes is in a separate group?


  bool zeroes;
-QLIST_ENTRY(BlockCopyTask) list;
+
+/* State */
  CoQueue wait_queue; /* coroutines blocked on this task */
+
+/* To reference all call states from BlockCopyState */
+QLIST_ENTRY(BlockCopyTask) list;
+


extra new-line?


  } BlockCopyTask;
  
  static int64_t task_end(BlockCopyTask *task)

@@ -89,15 +104,25 @@ typedef struct BlockCopyState {
   */
  BdrvChild *source;
  BdrvChild *target;
-BdrvDirtyBitmap *copy_bitmap;
+


you add an empty line before group, it looks good


+/* State */
  int64_t in_flight_bytes;
-int64_t cluster_size;
  BlockCopyMethod method;
-int64_t max_transfer;
-uint64_t len;
  QLIST_HEAD(, BlockCopyTask) tasks; /* All tasks from all block-copy calls 
*/
  QLIST_HEAD(, BlockCopyCallState) calls;


but not here..


+/* State fields that use a thread-safe API */
+BdrvDirtyBitmap *copy_bitmap;
+ProgressMeter *progress;
+SharedResource *mem;
+RateLimit rate_limit;
  
+/*

+ * IN parameters. Initialized in block_copy_state_new()
+ * and never changed.
+ */
+int64_t cluster_size;
+int64_t max_transfer;
+uint64_t len;
  BdrvRequestFlags write_flags;
  
  /*

@@ -115,12 +140,6 @@ typedef struct BlockCopyState {
   * block_copy_reset_unallocated() every time it does.
   */
  bool skip_unallocated;
-
-ProgressMeter *progress;
-
-SharedResource *mem;
-
-RateLimit rate_limit;
  } BlockCopyState;
  
  static BlockCopyTask *find_conflicting_task(BlockCopyState *s,

@@ -176,9 +195,9 @@ static inline int64_t block_copy_chunk_size(BlockCopyState 
*s)
   * Search for the first dirty area in offset/bytes range and create task at
   * the beginning of it.
   */
-static BlockCopyTask *block_copy_task_create(BlockCopyState *s,
- BlockCopyCallState *call_state,
- int64_t offset, int64_t bytes)
+static coroutine_fn BlockCopyTask *block_copy_task_create(BlockCopyState *s,
+BlockCopyCallState *call_state,
+int64_t offset, int64_t bytes)
  {
  BlockCopyTask *task;
  int64_t max_chunk = block_copy_chunk_size(s);




--
Best regards,
Vladimir

[PATCH 0/1] virtio: disable partitions scanning for no partitions block

[Yury Kamenev]

Some virtio blocks definitely have no partitions and should not be scanned.

Yury Kamenev (1):
  virtio: disable partitions scanning for no partitions block

 hw/block/virtio-blk.c   | 4 
 include/hw/virtio/virtio-blk.h  | 1 +
 include/standard-headers/linux/virtio_blk.h | 1 +
 3 files changed, 6 insertions(+)

-- 
2.24.3 (Apple Git-128)

[PATCH 0/1] virtio: disable partitions scanning for no partitions block

[Yury Kamenev]

Some virtio blocks definitely have no partitions and should not be scanned.

Yury Kamenev (1):
  virtio: disable partitions scanning for no partitions block

 hw/block/virtio-blk.c   | 4 
 include/hw/virtio/virtio-blk.h  | 1 +
 include/standard-headers/linux/virtio_blk.h | 1 +
 3 files changed, 6 insertions(+)

-- 
2.24.3 (Apple Git-128)

Re: [PATCH v2 3/7] block-copy: move progress_set_remaining in block_copy_task_end

[Vladimir Sementsov-Ogievskiy]

18.05.2021 13:07, Emanuele Giuseppe Esposito wrote:

Moving this function in task_end ensures to update the progress
anyways, even if there is an error.

It also helps in next patch, allowing task_end to have only
one critical section.

Signed-off-by: Emanuele Giuseppe Esposito 


Reviewed-by: Vladimir Sementsov-Ogievskiy 


---
  block/block-copy.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/block/block-copy.c b/block/block-copy.c
index d2d3839dec..2e610b4142 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -264,6 +264,9 @@ static void coroutine_fn block_copy_task_end(BlockCopyTask 
*task, int ret)
  bdrv_set_dirty_bitmap(task->s->copy_bitmap, task->offset, 
task->bytes);
  }
  QLIST_REMOVE(task, list);
+progress_set_remaining(task->s->progress,
+   bdrv_get_dirty_count(task->s->copy_bitmap) +
+   task->s->in_flight_bytes);
  qemu_co_queue_restart_all(&task->wait_queue);
  }
  
@@ -645,9 +648,6 @@ block_copy_dirty_clusters(BlockCopyCallState *call_state)

  }
  if (s->skip_unallocated && !(ret & BDRV_BLOCK_ALLOCATED)) {
  block_copy_task_end(task, 0);
-progress_set_remaining(s->progress,
-   bdrv_get_dirty_count(s->copy_bitmap) +
-   s->in_flight_bytes);
  trace_block_copy_skip_range(s, task->offset, task->bytes);
  offset = task_end(task);
  bytes = end - offset;




--
Best regards,
Vladimir

[PATCH v2 33/33] iotests/image-fleecing: add test-case for copy-before-write filter

[Vladimir Sementsov-Ogievskiy]

New fleecing method becomes available: copy-before-write filter.

Actually we don't need backup job to setup image fleecing. Add test
for new recommended way of image fleecing.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/tests/image-fleecing | 50 +-
 tests/qemu-iotests/tests/image-fleecing.out | 72 +
 2 files changed, 107 insertions(+), 15 deletions(-)

diff --git a/tests/qemu-iotests/tests/image-fleecing 
b/tests/qemu-iotests/tests/image-fleecing
index e210c00d28..404ebc00f1 100755
--- a/tests/qemu-iotests/tests/image-fleecing
+++ b/tests/qemu-iotests/tests/image-fleecing
@@ -48,7 +48,7 @@ remainder = [('0xd5', '0x108000',  '32k'), # Right-end of 
partial-left [1]
  ('0xdc', '32M',   '32k'), # Left-end of partial-right [2]
  ('0xcd', '0x3ff', '64k')] # patterns[3]
 
-def do_test(base_img_path, fleece_img_path, nbd_sock_path, vm):
+def do_test(use_cbw, base_img_path, fleece_img_path, nbd_sock_path, vm):
 log('--- Setting up images ---')
 log('')
 
@@ -67,6 +67,7 @@ def do_test(base_img_path, fleece_img_path, nbd_sock_path, 
vm):
 
 src_node = 'source'
 tmp_node = 'temp'
+qom_path = '/machine/peripheral/sda'
 vm.add_blockdev(f'driver={iotests.imgfmt},file.driver=file,'
 f'file.filename={base_img_path},node-name={src_node}')
 vm.add_device('virtio-scsi')
@@ -90,12 +91,22 @@ def do_test(base_img_path, fleece_img_path, nbd_sock_path, 
vm):
 'backing': src_node,
 }))
 
-# Establish COW from source to fleecing node
-log(vm.qmp('blockdev-backup',
-   job_id='fleecing',
-   device=src_node,
-   target=tmp_node,
-   sync='none'))
+# Establish CBW from source to fleecing node
+if use_cbw:
+log(vm.qmp('blockdev-add', **{
+'driver': 'copy-before-write',
+'node-name': 'fl-cbw',
+'file': src_node,
+'target': tmp_node
+}))
+
+log(vm.qmp('qom-set', path=qom_path, property='drive', value='fl-cbw'))
+else:
+log(vm.qmp('blockdev-backup',
+   job_id='fleecing',
+   device=src_node,
+   target=tmp_node,
+   sync='none'))
 
 log('')
 log('--- Setting up NBD Export ---')
@@ -124,7 +135,7 @@ def do_test(base_img_path, fleece_img_path, nbd_sock_path, 
vm):
 for p in overwrite:
 cmd = 'write -P%s %s %s' % p
 log(cmd)
-log(vm.hmp_qemu_io('/machine/peripheral/sda', cmd, qdev=True))
+log(vm.hmp_qemu_io(qom_path, cmd, qdev=True))
 
 log('')
 log('--- Verifying Data ---')
@@ -139,10 +150,15 @@ def do_test(base_img_path, fleece_img_path, 
nbd_sock_path, vm):
 log('--- Cleanup ---')
 log('')
 
-log(vm.qmp('block-job-cancel', device='fleecing'))
-e = vm.event_wait('BLOCK_JOB_CANCELLED')
-assert e is not None
-log(e, filters=[iotests.filter_qmp_event])
+if use_cbw:
+log(vm.qmp('qom-set', path=qom_path, property='drive', value=src_node))
+log(vm.qmp('blockdev-del', node_name='fl-cbw'))
+else:
+log(vm.qmp('block-job-cancel', device='fleecing'))
+e = vm.event_wait('BLOCK_JOB_CANCELLED')
+assert e is not None
+log(e, filters=[iotests.filter_qmp_event])
+
 log(vm.qmp('nbd-server-stop'))
 log(vm.qmp('blockdev-del', node_name=tmp_node))
 vm.shutdown()
@@ -160,13 +176,17 @@ def do_test(base_img_path, fleece_img_path, 
nbd_sock_path, vm):
 log('Done')
 
 
-def test():
+def test(use_cbw):
 with iotests.FilePath('base.img') as base_img_path, \
  iotests.FilePath('fleece.img') as fleece_img_path, \
  iotests.FilePath('nbd.sock',
   base_dir=iotests.sock_dir) as nbd_sock_path, \
  iotests.VM() as vm:
-do_test(base_img_path, fleece_img_path, nbd_sock_path, vm)
+do_test(use_cbw, base_img_path, fleece_img_path, nbd_sock_path, vm)
+
 
+log('=== Test backup(sync=none) based fleecing ===\n')
+test(False)
 
-test()
+log('=== Test filter based fleecing ===\n')
+test(True)
diff --git a/tests/qemu-iotests/tests/image-fleecing.out 
b/tests/qemu-iotests/tests/image-fleecing.out
index 314a1b54e9..e96d122a8b 100644
--- a/tests/qemu-iotests/tests/image-fleecing.out
+++ b/tests/qemu-iotests/tests/image-fleecing.out
@@ -1,3 +1,5 @@
+=== Test backup(sync=none) based fleecing ===
+
 --- Setting up images ---
 
 Done
@@ -65,3 +67,73 @@ read -P0xdc 32M 32k
 read -P0xcd 0x3ff 64k
 
 Done
+=== Test filter based fleecing ===
+
+--- Setting up images ---
+
+Done
+
+--- Launching VM ---
+
+Done
+
+--- Setting up Fleecing Graph ---
+
+{"return": {}}
+{"return": {}}
+{"return": {}}
+
+--- Setting up NBD Export ---
+
+{"return": {}}
+{"return": {}}
+
+--- Sanity Check ---
+
+read -P0x5d 0 64k
+read -P0xd5 1M 64k
+read -P0xdc 32M 64k
+read -P0xcd 0x3ff 64k
+read -P0 0x00f8000 32k
+read -P0 0x201 32k

Re: [PATCH v2 0/7] block-copy: protect block-copy internal structures

[Emanuele Giuseppe Esposito]

On 20/05/2021 15:47, Vladimir Sementsov-Ogievskiy wrote:

18.05.2021 13:07, Emanuele Giuseppe Esposito wrote:

This serie of patches aims to reduce the usage of the global
AioContexlock in block-copy, by introducing smaller granularity
locks thus on making the block layer thread safe.

This serie depends on Paolo's coroutine_sleep API and my previous
serie that brings thread safety to the smaller API used by block-copy,
like ratelimit, progressmeter abd co-shared-resource.

What's missing for block-copy to be fully thread-safe is fixing
the CoSleep API to allow cross-thread sleep and wakeup.
Paolo is working on it and will post the patches once his new
CoSleep API is accepted.

Patch 1 introduces the .method field instead of .use_copy_range
and .copy_size, so that it can be later used as atomic.
Patch 2-3 provide comments and refactoring in preparation to
the locks added in patch 4 on BlockCopyTask, patch 5-6 on
BlockCopyCallState and 7 BlockCopyState.

Based-on: <20210517100548.28806-1-pbonz...@redhat.com>
Based-on: <20210518094058.25952-1-eespo...@redhat.com>


Hi! I failed to apply this all. Could you please export your branch with 
your patches at some public git repo?


Hi, thank you for applying the patches. My branch is here:

https://gitlab.com/eesposit/qemu/-/commits/dataplane_new

Emanuele



Signed-off-by: Emanuele Giuseppe Esposito 
---
v1 -> v2:
* More field categorized as IN/State/OUT in the various struct, better
   documentation in the structs
* Fix a couple of places where I missed locks [Vladimir, Paolo]


Emanuele Giuseppe Esposito (6):
   block-copy: improve documentation of BlockCopyTask and BlockCopyState
 types and functions
   block-copy: move progress_set_remaining in block_copy_task_end
   block-copy: add a CoMutex to the BlockCopyTask list
   block-copy: add QemuMutex lock for BlockCopyCallState list
   block-copy: atomic .cancelled and .finished fields in
 BlockCopyCallState
   block-copy: protect BlockCopyState .method fields

Paolo Bonzini (1):
   block-copy: streamline choice of copy_range vs. read/write

  block/block-copy.c | 234 +
  1 file changed, 150 insertions(+), 84 deletions(-)

[PATCH v2 22/33] qapi: publish copy-before-write filter

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 qapi/block-core.json | 30 ++
 1 file changed, 26 insertions(+), 4 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 2ea294129e..8c4801a13d 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2808,15 +2808,17 @@
 # @blklogwrites: Since 3.0
 # @blkreplay: Since 4.2
 # @compress: Since 5.0
+# @copy-before-write: Since 6.1
 #
 # Since: 2.9
 ##
 { 'enum': 'BlockdevDriver',
   'data': [ 'blkdebug', 'blklogwrites', 'blkreplay', 'blkverify', 'bochs',
-'cloop', 'compress', 'copy-on-read', 'dmg', 'file', 'ftp', 'ftps',
-'gluster', 'host_cdrom', 'host_device', 'http', 'https', 'iscsi',
-'luks', 'nbd', 'nfs', 'null-aio', 'null-co', 'nvme', 'parallels',
-'preallocate', 'qcow', 'qcow2', 'qed', 'quorum', 'raw', 'rbd',
+'cloop', 'compress', 'copy-before-write', 'copy-on-read', 'dmg',
+'file', 'ftp', 'ftps', 'gluster', 'host_cdrom', 'host_device',
+'http', 'https', 'iscsi', 'luks', 'nbd', 'nfs', 'null-aio',
+'null-co', 'nvme', 'parallels', 'preallocate', 'qcow', 'qcow2',
+'qed', 'quorum', 'raw', 'rbd',
 { 'name': 'replication', 'if': 'defined(CONFIG_REPLICATION)' },
 'ssh', 'throttle', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] }
 
@@ -3937,6 +3939,25 @@
   'base': 'BlockdevOptionsGenericFormat',
   'data': { '*bottom': 'str' } }
 
+##
+# @BlockdevOptionsCbw:
+#
+# Driver specific block device options for the copy-before-write driver,
+# which does so called copy-before-write operations: when data is
+# written to the filter, the filter firstly reads corresponding blocks
+# from its file child and copies them to @target child. After successful
+# copying the write request is propagated to file child. If copying
+# filed, the original write request is failed too and no data is written
+# to file child.
+#
+# @target: The target for copy-before-write operations.
+#
+# Since: 6.1
+##
+{ 'struct': 'BlockdevOptionsCbw',
+  'base': 'BlockdevOptionsGenericFormat',
+  'data': { 'target': 'BlockdevRef' } }
+
 ##
 # @BlockdevOptions:
 #
@@ -3989,6 +4010,7 @@
   'bochs':  'BlockdevOptionsGenericFormat',
   'cloop':  'BlockdevOptionsGenericFormat',
   'compress':   'BlockdevOptionsGenericFormat',
+  'copy-before-write':'BlockdevOptionsCbw',
   'copy-on-read':'BlockdevOptionsCor',
   'dmg':'BlockdevOptionsGenericFormat',
   'file':   'BlockdevOptionsFile',
-- 
2.29.2

[PATCH v2 32/33] iotests/image-fleecing: prepare for adding new test-case

[Vladimir Sementsov-Ogievskiy]

We are going to add a test-case with some behavior modifications. So,
let's prepare a function to be reused.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/tests/image-fleecing | 19 +--
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/tests/qemu-iotests/tests/image-fleecing 
b/tests/qemu-iotests/tests/image-fleecing
index ec4ef5f3f6..e210c00d28 100755
--- a/tests/qemu-iotests/tests/image-fleecing
+++ b/tests/qemu-iotests/tests/image-fleecing
@@ -48,12 +48,7 @@ remainder = [('0xd5', '0x108000',  '32k'), # Right-end of 
partial-left [1]
  ('0xdc', '32M',   '32k'), # Left-end of partial-right [2]
  ('0xcd', '0x3ff', '64k')] # patterns[3]
 
-with iotests.FilePath('base.img') as base_img_path, \
- iotests.FilePath('fleece.img') as fleece_img_path, \
- iotests.FilePath('nbd.sock',
-  base_dir=iotests.sock_dir) as nbd_sock_path, \
- iotests.VM() as vm:
-
+def do_test(base_img_path, fleece_img_path, nbd_sock_path, vm):
 log('--- Setting up images ---')
 log('')
 
@@ -163,3 +158,15 @@ with iotests.FilePath('base.img') as base_img_path, \
 
 log('')
 log('Done')
+
+
+def test():
+with iotests.FilePath('base.img') as base_img_path, \
+ iotests.FilePath('fleece.img') as fleece_img_path, \
+ iotests.FilePath('nbd.sock',
+  base_dir=iotests.sock_dir) as nbd_sock_path, \
+ iotests.VM() as vm:
+do_test(base_img_path, fleece_img_path, nbd_sock_path, vm)
+
+
+test()
-- 
2.29.2

[PATCH v2 21/33] block/copy-before-write: make public block driver

[Vladimir Sementsov-Ogievskiy]

Finally, copy-before-write gets own .bdrv_open and .bdrv_close
handlers, block_init() call and becomes available through bdrv_open().

To achieve this:

 - cbw_init gets unused flags argument and becomes cbw_open
 - block_copy_state_free() call moved to new cbw_close()
 - in bdrv_cbw_append:
   - options are completed with driver and node-name, and we can simply
 use bdrv_insert_node() to do both open and drained replacing
 - in bdrv_cbw_drop:
   - cbw_close() is now responsible for freeing s->bcs, so don't do it
 here

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
---
 block/copy-before-write.c | 60 ++-
 1 file changed, 28 insertions(+), 32 deletions(-)

diff --git a/block/copy-before-write.c b/block/copy-before-write.c
index ba2466a328..6ed5bce1f1 100644
--- a/block/copy-before-write.c
+++ b/block/copy-before-write.c
@@ -144,7 +144,8 @@ static void cbw_child_perm(BlockDriverState *bs, BdrvChild 
*c,
 }
 }
 
-static int cbw_init(BlockDriverState *bs, QDict *options, Error **errp)
+static int cbw_open(BlockDriverState *bs, QDict *options, int flags,
+Error **errp)
 {
 BDRVCopyBeforeWriteState *s = bs->opaque;
 BdrvDirtyBitmap *copy_bitmap;
@@ -191,10 +192,21 @@ static int cbw_init(BlockDriverState *bs, QDict *options, 
Error **errp)
 return 0;
 }
 
+static void cbw_close(BlockDriverState *bs)
+{
+BDRVCopyBeforeWriteState *s = bs->opaque;
+
+block_copy_state_free(s->bcs);
+s->bcs = NULL;
+}
+
 BlockDriver bdrv_cbw_filter = {
 .format_name = "copy-before-write",
 .instance_size = sizeof(BDRVCopyBeforeWriteState),
 
+.bdrv_open  = cbw_open,
+.bdrv_close = cbw_close,
+
 .bdrv_co_preadv = cbw_co_preadv,
 .bdrv_co_pwritev= cbw_co_pwritev,
 .bdrv_co_pwrite_zeroes  = cbw_co_pwrite_zeroes,
@@ -216,57 +228,41 @@ BlockDriverState *bdrv_cbw_append(BlockDriverState 
*source,
   Error **errp)
 {
 ERRP_GUARD();
-int ret;
 BDRVCopyBeforeWriteState *state;
 BlockDriverState *top;
 QDict *opts;
 
 assert(source->total_sectors == target->total_sectors);
 
-top = bdrv_new_open_driver(&bdrv_cbw_filter, filter_node_name,
-   BDRV_O_RDWR, errp);
-if (!top) {
-error_prepend(errp, "Cannot open driver: ");
-return NULL;
-}
-state = top->opaque;
-
 opts = qdict_new();
+qdict_put_str(opts, "driver", "copy-before-write");
+if (filter_node_name) {
+qdict_put_str(opts, "node-name", filter_node_name);
+}
 qdict_put_str(opts, "file", bdrv_get_node_name(source));
 qdict_put_str(opts, "target", bdrv_get_node_name(target));
 qdict_put_bool(opts, "x-deprecated-compress", compress);
 
-ret = cbw_init(top, opts, errp);
-qobject_unref(opts);
-if (ret < 0) {
-goto fail;
-}
-
-bdrv_drained_begin(source);
-ret = bdrv_replace_node(source, top, errp);
-bdrv_drained_end(source);
-if (ret < 0) {
-error_prepend(errp, "Cannot append copy-before-write filter: ");
-goto fail;
+top = bdrv_insert_node(source, opts, BDRV_O_RDWR, errp);
+if (!top) {
+return NULL;
 }
 
+state = top->opaque;
 *bcs = state->bcs;
 
 return top;
-
-fail:
-block_copy_state_free(state->bcs);
-bdrv_unref(top);
-return NULL;
 }
 
 void bdrv_cbw_drop(BlockDriverState *bs)
 {
-BDRVCopyBeforeWriteState *s = bs->opaque;
-
 bdrv_drop_filter(bs, &error_abort);
-
-block_copy_state_free(s->bcs);
-
 bdrv_unref(bs);
 }
+
+static void cbw_init(void)
+{
+bdrv_register(&bdrv_cbw_filter);
+}
+
+block_init(cbw_init);
-- 
2.29.2

Re: [PATCH v2 1/7] block-copy: streamline choice of copy_range vs. read/write

[Vladimir Sementsov-Ogievskiy]

18.05.2021 13:07, Emanuele Giuseppe Esposito wrote:

From: Paolo Bonzini 

Put the logic to determine the copy size in a separate function, so
that there is a simple state machine for the possible methods of
copying data from one BlockDriverState to the other.


Honestly, for me 4-state state-maching + function to determine copy-size 
doesn't seem better than two simple variables copy_size and use_copy_range.

What's the benefit of it?



While at it, store the common computation of block_copy_max_transfer
into a new field of BlockCopyState, and make sure that we always
obey max_transfer; that's more efficient even for the
COPY_RANGE_READ_WRITE case.


hmm, maybe. It could be a separate patch.



Signed-off-by: Emanuele Giuseppe Esposito 
Signed-off-by: Paolo Bonzini 
---
  block/block-copy.c | 59 ++


stats agree with me, that its' not a simplification.


  1 file changed, 39 insertions(+), 20 deletions(-)

diff --git a/block/block-copy.c b/block/block-copy.c
index 37c8e8504b..10ce51a244 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -28,6 +28,13 @@
  #define BLOCK_COPY_MAX_WORKERS 64
  #define BLOCK_COPY_SLICE_TIME 1ULL /* ns */
  
+typedef enum {

+COPY_READ_WRITE_CLUSTER,
+COPY_READ_WRITE,
+COPY_RANGE_SMALL,
+COPY_RANGE_FULL
+} BlockCopyMethod;
+
  static coroutine_fn int block_copy_task_entry(AioTask *task);
  
  typedef struct BlockCopyCallState {

@@ -85,8 +92,8 @@ typedef struct BlockCopyState {
  BdrvDirtyBitmap *copy_bitmap;
  int64_t in_flight_bytes;
  int64_t cluster_size;
-bool use_copy_range;
-int64_t copy_size;
+BlockCopyMethod method;
+int64_t max_transfer;
  uint64_t len;
  QLIST_HEAD(, BlockCopyTask) tasks; /* All tasks from all block-copy calls 
*/
  QLIST_HEAD(, BlockCopyCallState) calls;
@@ -148,6 +155,23 @@ static bool coroutine_fn 
block_copy_wait_one(BlockCopyState *s, int64_t offset,
  return true;
  }
  
+static inline int64_t block_copy_chunk_size(BlockCopyState *s)


"inline" word does nothing in static definitions in c files. Compiler make a 
decision independently of it.


+{
+switch (s->method) {
+case COPY_READ_WRITE_CLUSTER:
+return s->cluster_size;
+case COPY_READ_WRITE:
+case COPY_RANGE_SMALL:
+return MIN(MAX(s->cluster_size, BLOCK_COPY_MAX_BUFFER),
+   s->max_transfer);
+case COPY_RANGE_FULL:
+return MIN(MAX(s->cluster_size, BLOCK_COPY_MAX_COPY_RANGE),
+   s->max_transfer);
+default:
+abort();
+}
+}
+
  /*
   * Search for the first dirty area in offset/bytes range and create task at
   * the beginning of it.
@@ -157,8 +181,9 @@ static BlockCopyTask *block_copy_task_create(BlockCopyState 
*s,
   int64_t offset, int64_t bytes)
  {
  BlockCopyTask *task;
-int64_t max_chunk = MIN_NON_ZERO(s->copy_size, call_state->max_chunk);
+int64_t max_chunk = block_copy_chunk_size(s);
  
+max_chunk = MIN_NON_ZERO(max_chunk, call_state->max_chunk);

  if (!bdrv_dirty_bitmap_next_dirty_area(s->copy_bitmap,
 offset, offset + bytes,
 max_chunk, &offset, &bytes))
@@ -265,28 +290,27 @@ BlockCopyState *block_copy_state_new(BdrvChild *source, 
BdrvChild *target,
  .len = bdrv_dirty_bitmap_size(copy_bitmap),
  .write_flags = write_flags,
  .mem = shres_create(BLOCK_COPY_MAX_MEM),
+.max_transfer = QEMU_ALIGN_DOWN(block_copy_max_transfer(source, target)
+, cluster_size),
  };
  
-if (block_copy_max_transfer(source, target) < cluster_size) {

+if (s->max_transfer < cluster_size) {
  /*
   * copy_range does not respect max_transfer. We don't want to bother
   * with requests smaller than block-copy cluster size, so fallback to
   * buffered copying (read and write respect max_transfer on their
   * behalf).
   */
-s->use_copy_range = false;
-s->copy_size = cluster_size;
+s->method = COPY_READ_WRITE_CLUSTER;
  } else if (write_flags & BDRV_REQ_WRITE_COMPRESSED) {
  /* Compression supports only cluster-size writes and no copy-range. */
-s->use_copy_range = false;
-s->copy_size = cluster_size;
+s->method = COPY_READ_WRITE_CLUSTER;
  } else {
  /*
   * We enable copy-range, but keep small copy_size, until first
   * successful copy_range (look at block_copy_do_copy).
   */
-s->use_copy_range = use_copy_range;
-s->copy_size = MAX(s->cluster_size, BLOCK_COPY_MAX_BUFFER);
+s->method = use_copy_range ? COPY_RANGE_SMALL : COPY_READ_WRITE;
  }
  
  ratelimit_init(&s->rate_limit);

@@ -369,30 +393,25 @@ static int coroutine_fn block_copy_do_copy(BlockCopyState 
*s,
  return ret;
  

[PATCH v2 31/33] iotests/image-fleecing: rename tgt_node

[Vladimir Sementsov-Ogievskiy]

Actually target of backup(sync=None) is not a final backup target:
image fleecing is intended to be used with external tool, which will
copy data from fleecing node to some real backup target.

Also, we are going to add a test case for "push backup with fleecing",
where instead of exporting fleecing node by NBD, we'll start a backup
job from fleecing node to real backup target.

To avoid confusion, let's rename temporary fleecing node now.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/tests/image-fleecing | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/tests/qemu-iotests/tests/image-fleecing 
b/tests/qemu-iotests/tests/image-fleecing
index 961941bb27..ec4ef5f3f6 100755
--- a/tests/qemu-iotests/tests/image-fleecing
+++ b/tests/qemu-iotests/tests/image-fleecing
@@ -71,6 +71,7 @@ with iotests.FilePath('base.img') as base_img_path, \
 log('')
 
 src_node = 'source'
+tmp_node = 'temp'
 vm.add_blockdev(f'driver={iotests.imgfmt},file.driver=file,'
 f'file.filename={base_img_path},node-name={src_node}')
 vm.add_device('virtio-scsi')
@@ -82,12 +83,11 @@ with iotests.FilePath('base.img') as base_img_path, \
 log('--- Setting up Fleecing Graph ---')
 log('')
 
-tgt_node = 'fleeceNode'
 
-# create tgt_node backed by src_node
+# create tmp_node backed by src_node
 log(vm.qmp('blockdev-add', {
 'driver': 'qcow2',
-'node-name': tgt_node,
+'node-name': tmp_node,
 'file': {
 'driver': 'file',
 'filename': fleece_img_path,
@@ -99,19 +99,19 @@ with iotests.FilePath('base.img') as base_img_path, \
 log(vm.qmp('blockdev-backup',
job_id='fleecing',
device=src_node,
-   target=tgt_node,
+   target=tmp_node,
sync='none'))
 
 log('')
 log('--- Setting up NBD Export ---')
 log('')
 
-nbd_uri = 'nbd+unix:///%s?socket=%s' % (tgt_node, nbd_sock_path)
+nbd_uri = 'nbd+unix:///%s?socket=%s' % (tmp_node, nbd_sock_path)
 log(vm.qmp('nbd-server-start',
{'addr': { 'type': 'unix',
   'data': { 'path': nbd_sock_path } } }))
 
-log(vm.qmp('nbd-server-add', device=tgt_node))
+log(vm.qmp('nbd-server-add', device=tmp_node))
 
 log('')
 log('--- Sanity Check ---')
@@ -149,7 +149,7 @@ with iotests.FilePath('base.img') as base_img_path, \
 assert e is not None
 log(e, filters=[iotests.filter_qmp_event])
 log(vm.qmp('nbd-server-stop'))
-log(vm.qmp('blockdev-del', node_name=tgt_node))
+log(vm.qmp('blockdev-del', node_name=tmp_node))
 vm.shutdown()
 
 log('')
-- 
2.29.2

[PATCH v2 20/33] block/block-copy: make setting progress optional

[Vladimir Sementsov-Ogievskiy]

Now block-copy will crash if user don't set progress meter by
block_copy_set_progress_meter(). copy-before-write filter will be used
in separate of backup job, and it doesn't want any progress meter (for
now). So, allow not setting it.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
---
 block/block-copy.c | 18 +++---
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/block/block-copy.c b/block/block-copy.c
index 9389c7c6c8..0a9c3692bf 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -509,7 +509,7 @@ static coroutine_fn int block_copy_task_entry(AioTask *task)
 if (ret < 0 && !t->call_state->ret) {
 t->call_state->ret = ret;
 t->call_state->error_is_read = error_is_read;
-} else {
+} else if (t->s->progress) {
 progress_work_done(t->s->progress, t->bytes);
 }
 co_put_to_shres(t->s->mem, t->bytes);
@@ -613,9 +613,11 @@ int64_t block_copy_reset_unallocated(BlockCopyState *s,
 
 if (!ret) {
 bdrv_reset_dirty_bitmap(s->copy_bitmap, offset, bytes);
-progress_set_remaining(s->progress,
-   bdrv_get_dirty_count(s->copy_bitmap) +
-   s->in_flight_bytes);
+if (s->progress) {
+progress_set_remaining(s->progress,
+   bdrv_get_dirty_count(s->copy_bitmap) +
+   s->in_flight_bytes);
+}
 }
 
 *count = bytes;
@@ -675,9 +677,11 @@ block_copy_dirty_clusters(BlockCopyCallState *call_state)
 }
 if (s->skip_unallocated && !(ret & BDRV_BLOCK_ALLOCATED)) {
 block_copy_task_end(task, 0);
-progress_set_remaining(s->progress,
-   bdrv_get_dirty_count(s->copy_bitmap) +
-   s->in_flight_bytes);
+if (s->progress) {
+progress_set_remaining(s->progress,
+   bdrv_get_dirty_count(s->copy_bitmap) +
+   s->in_flight_bytes);
+}
 trace_block_copy_skip_range(s, task->offset, task->bytes);
 offset = task_end(task);
 bytes = end - offset;
-- 
2.29.2

[PATCH v2 17/33] block/copy-before-write: cbw_init(): use file child after attaching

[Vladimir Sementsov-Ogievskiy]

In the next commit we'll get rid of source argument of cbw_init().
Prepare to it now, to make next commit simpler: move the code block
that uses source below attaching the child and use bs->file->bs instead
of source variable.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
---
 block/copy-before-write.c | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/block/copy-before-write.c b/block/copy-before-write.c
index d7f1833efa..4858dcf8ff 100644
--- a/block/copy-before-write.c
+++ b/block/copy-before-write.c
@@ -149,13 +149,6 @@ static int cbw_init(BlockDriverState *bs, BlockDriverState 
*source,
 {
 BDRVCopyBeforeWriteState *s = bs->opaque;
 
-bs->total_sectors = source->total_sectors;
-bs->supported_write_flags = BDRV_REQ_WRITE_UNCHANGED |
-(BDRV_REQ_FUA & source->supported_write_flags);
-bs->supported_zero_flags = BDRV_REQ_WRITE_UNCHANGED |
-((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) &
- source->supported_zero_flags);
-
 bdrv_ref(target);
 s->target = bdrv_attach_child(bs, target, "target", &child_of_bds,
   BDRV_CHILD_DATA, errp);
@@ -173,6 +166,13 @@ static int cbw_init(BlockDriverState *bs, BlockDriverState 
*source,
 return -EINVAL;
 }
 
+bs->total_sectors = bs->file->bs->total_sectors;
+bs->supported_write_flags = BDRV_REQ_WRITE_UNCHANGED |
+(BDRV_REQ_FUA & bs->file->bs->supported_write_flags);
+bs->supported_zero_flags = BDRV_REQ_WRITE_UNCHANGED |
+((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) &
+ bs->file->bs->supported_zero_flags);
+
 s->bcs = block_copy_state_new(bs->file, s->target, false, compress, errp);
 if (!s->bcs) {
 error_prepend(errp, "Cannot create block-copy-state: ");
-- 
2.29.2

[PATCH v2 30/33] iotests/image-fleecing: proper source device

[Vladimir Sementsov-Ogievskiy]

Define scsi device to operate with it by qom-set in further patch.

Give a new node-name to source block node, to not look like device
name.

Job now don't want to work without giving explicit id, so, let's call
it "fleecing".

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/tests/image-fleecing | 12 
 tests/qemu-iotests/tests/image-fleecing.out |  2 +-
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/tests/qemu-iotests/tests/image-fleecing 
b/tests/qemu-iotests/tests/image-fleecing
index 799369e290..961941bb27 100755
--- a/tests/qemu-iotests/tests/image-fleecing
+++ b/tests/qemu-iotests/tests/image-fleecing
@@ -70,7 +70,11 @@ with iotests.FilePath('base.img') as base_img_path, \
 log('--- Launching VM ---')
 log('')
 
-vm.add_drive(base_img_path)
+src_node = 'source'
+vm.add_blockdev(f'driver={iotests.imgfmt},file.driver=file,'
+f'file.filename={base_img_path},node-name={src_node}')
+vm.add_device('virtio-scsi')
+vm.add_device(f'scsi-hd,id=sda,drive={src_node}')
 vm.launch()
 log('Done')
 
@@ -78,7 +82,6 @@ with iotests.FilePath('base.img') as base_img_path, \
 log('--- Setting up Fleecing Graph ---')
 log('')
 
-src_node = 'drive0'
 tgt_node = 'fleeceNode'
 
 # create tgt_node backed by src_node
@@ -94,6 +97,7 @@ with iotests.FilePath('base.img') as base_img_path, \
 
 # Establish COW from source to fleecing node
 log(vm.qmp('blockdev-backup',
+   job_id='fleecing',
device=src_node,
target=tgt_node,
sync='none'))
@@ -125,7 +129,7 @@ with iotests.FilePath('base.img') as base_img_path, \
 for p in overwrite:
 cmd = 'write -P%s %s %s' % p
 log(cmd)
-log(vm.hmp_qemu_io(src_node, cmd))
+log(vm.hmp_qemu_io('/machine/peripheral/sda', cmd, qdev=True))
 
 log('')
 log('--- Verifying Data ---')
@@ -140,7 +144,7 @@ with iotests.FilePath('base.img') as base_img_path, \
 log('--- Cleanup ---')
 log('')
 
-log(vm.qmp('block-job-cancel', device=src_node))
+log(vm.qmp('block-job-cancel', device='fleecing'))
 e = vm.event_wait('BLOCK_JOB_CANCELLED')
 assert e is not None
 log(e, filters=[iotests.filter_qmp_event])
diff --git a/tests/qemu-iotests/tests/image-fleecing.out 
b/tests/qemu-iotests/tests/image-fleecing.out
index 16643dde30..314a1b54e9 100644
--- a/tests/qemu-iotests/tests/image-fleecing.out
+++ b/tests/qemu-iotests/tests/image-fleecing.out
@@ -50,7 +50,7 @@ read -P0 0x3fe 64k
 --- Cleanup ---
 
 {"return": {}}
-{"data": {"device": "drive0", "len": 67108864, "offset": 393216, "speed": 0, 
"type": "backup"}, "event": "BLOCK_JOB_CANCELLED", "timestamp": 
{"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"device": "fleecing", "len": 67108864, "offset": 393216, "speed": 0, 
"type": "backup"}, "event": "BLOCK_JOB_CANCELLED", "timestamp": 
{"microseconds": "USECS", "seconds": "SECS"}}
 {"return": {}}
 {"return": {}}
 
-- 
2.29.2

[PATCH v2 24/33] python/qemu/machine: QEMUMachine: improve qmp() method

[Vladimir Sementsov-Ogievskiy]

We often call qmp() with unpacking dict, like qmp('foo', **{...}).
mypy don't really like it, it things that passed unpacked dict is a
positional argument and complains that it type should be bool (because
second argument of qmp() is conv_keys: bool).

Allow possing dict directly, simplifying interface, and giving a way to
satisfy mypy.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 python/qemu/machine.py | 12 +++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index ff35f2cd6c..7a14605040 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -549,11 +549,21 @@ def _qmp_args(cls, conv_keys: bool,
 return args
 
 def qmp(self, cmd: str,
-conv_keys: bool = True,
+args_dict: Optional[Dict[str, Any]] = None,
+conv_keys: Optional[bool] = None,
 **args: Any) -> QMPMessage:
 """
 Invoke a QMP command and return the response dict
 """
+if args_dict is not None:
+assert not args
+assert conv_keys is None
+args = args_dict
+conv_keys = False
+
+if conv_keys is None:
+conv_keys = True
+
 qmp_args = self._qmp_args(conv_keys, args)
 return self._qmp.cmd(cmd, args=qmp_args)
 
-- 
2.29.2

[PATCH v2 27/33] iotests/222: constantly use single quotes for strings

[Vladimir Sementsov-Ogievskiy]

The file use both single and double quotes for strings. Let's be
consistent.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/222 | 68 +-
 1 file changed, 34 insertions(+), 34 deletions(-)

diff --git a/tests/qemu-iotests/222 b/tests/qemu-iotests/222
index 5e2556f8df..799369e290 100755
--- a/tests/qemu-iotests/222
+++ b/tests/qemu-iotests/222
@@ -30,23 +30,23 @@ iotests.script_initialize(
 supported_platforms=['linux'],
 )
 
-patterns = [("0x5d", "0", "64k"),
-("0xd5", "1M","64k"),
-("0xdc", "32M",   "64k"),
-("0xcd", "0x3ff", "64k")]  # 64M - 64K
+patterns = [('0x5d', '0', '64k'),
+('0xd5', '1M','64k'),
+('0xdc', '32M',   '64k'),
+('0xcd', '0x3ff', '64k')]  # 64M - 64K
 
-overwrite = [("0xab", "0", "64k"), # Full overwrite
- ("0xad", "0x00f8000", "64k"), # Partial-left (1M-32K)
- ("0x1d", "0x2008000", "64k"), # Partial-right (32M+32K)
- ("0xea", "0x3fe", "64k")] # Adjacent-left (64M - 128K)
+overwrite = [('0xab', '0', '64k'), # Full overwrite
+ ('0xad', '0x00f8000', '64k'), # Partial-left (1M-32K)
+ ('0x1d', '0x2008000', '64k'), # Partial-right (32M+32K)
+ ('0xea', '0x3fe', '64k')] # Adjacent-left (64M - 128K)
 
-zeroes = [("0", "0x00f8000", "32k"), # Left-end of partial-left (1M-32K)
-  ("0", "0x201", "32k"), # Right-end of partial-right (32M+64K)
-  ("0", "0x3fe", "64k")] # overwrite[3]
+zeroes = [('0', '0x00f8000', '32k'), # Left-end of partial-left (1M-32K)
+  ('0', '0x201', '32k'), # Right-end of partial-right (32M+64K)
+  ('0', '0x3fe', '64k')] # overwrite[3]
 
-remainder = [("0xd5", "0x108000",  "32k"), # Right-end of partial-left [1]
- ("0xdc", "32M",   "32k"), # Left-end of partial-right [2]
- ("0xcd", "0x3ff", "64k")] # patterns[3]
+remainder = [('0xd5', '0x108000',  '32k'), # Right-end of partial-left [1]
+ ('0xdc', '32M',   '32k'), # Left-end of partial-right [2]
+ ('0xcd', '0x3ff', '64k')] # patterns[3]
 
 with iotests.FilePath('base.img') as base_img_path, \
  iotests.FilePath('fleece.img') as fleece_img_path, \
@@ -58,7 +58,7 @@ with iotests.FilePath('base.img') as base_img_path, \
 log('')
 
 assert qemu_img('create', '-f', iotests.imgfmt, base_img_path, '64M') == 0
-assert qemu_img('create', '-f', "qcow2", fleece_img_path, '64M') == 0
+assert qemu_img('create', '-f', 'qcow2', fleece_img_path, '64M') == 0
 
 for p in patterns:
 qemu_io('-f', iotests.imgfmt,
@@ -78,43 +78,43 @@ with iotests.FilePath('base.img') as base_img_path, \
 log('--- Setting up Fleecing Graph ---')
 log('')
 
-src_node = "drive0"
-tgt_node = "fleeceNode"
+src_node = 'drive0'
+tgt_node = 'fleeceNode'
 
 # create tgt_node backed by src_node
-log(vm.qmp("blockdev-add", {
-"driver": "qcow2",
-"node-name": tgt_node,
-"file": {
-"driver": "file",
-"filename": fleece_img_path,
+log(vm.qmp('blockdev-add', {
+'driver': 'qcow2',
+'node-name': tgt_node,
+'file': {
+'driver': 'file',
+'filename': fleece_img_path,
 },
-"backing": src_node,
+'backing': src_node,
 }))
 
 # Establish COW from source to fleecing node
-log(vm.qmp("blockdev-backup",
+log(vm.qmp('blockdev-backup',
device=src_node,
target=tgt_node,
-   sync="none"))
+   sync='none'))
 
 log('')
 log('--- Setting up NBD Export ---')
 log('')
 
 nbd_uri = 'nbd+unix:///%s?socket=%s' % (tgt_node, nbd_sock_path)
-log(vm.qmp("nbd-server-start",
-   {"addr": { "type": "unix",
-  "data": { "path": nbd_sock_path } } }))
+log(vm.qmp('nbd-server-start',
+   {'addr': { 'type': 'unix',
+  'data': { 'path': nbd_sock_path } } }))
 
-log(vm.qmp("nbd-server-add", device=tgt_node))
+log(vm.qmp('nbd-server-add', device=tgt_node))
 
 log('')
 log('--- Sanity Check ---')
 log('')
 
 for p in patterns + zeroes:
-cmd = "read -P%s %s %s" % p
+cmd = 'read -P%s %s %s' % p
 log(cmd)
 assert qemu_io_silent('-r', '-f', 'raw', '-c', cmd, nbd_uri) == 0
 
@@ -123,7 +123,7 @@ with iotests.FilePath('base.img') as base_img_path, \
 log('')
 
 for p in overwrite:
-cmd = "write -P%s %s %s" % p
+cmd = 'write -P%s %s %s' % p
 log(cmd)
 log(vm.hmp_qemu_io(src_node, cmd))
 
@@ -132,7 +132,7 @@ with iotests.FilePath('base.img') as base_img_path, \
 log('')
 
 for p in patterns + zeroes:
-cmd = "read -P%s %s %s" % p
+cmd = 'read -P%s %s %s' % p
 log(cmd

[PATCH v2 29/33] iotests.py: hmp_qemu_io: support qdev

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/iotests.py | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index c7ec16c082..9fa0162b07 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -655,9 +655,10 @@ def resume_drive(self, drive: str) -> None:
 self.hmp(f'qemu-io {drive} "remove_break bp_{drive}"')
 
 def hmp_qemu_io(self, drive: str, cmd: str,
-use_log: bool = False) -> QMPMessage:
+use_log: bool = False, qdev: bool = False) -> QMPMessage:
 """Write to a given drive using an HMP command"""
-return self.hmp(f'qemu-io {drive} "{cmd}"', use_log=use_log)
+d = '-d ' if qdev else ''
+return self.hmp(f'qemu-io {d}{drive} "{cmd}"', use_log=use_log)
 
 def flatten_qmp_object(self, obj, output=None, basestr=''):
 if output is None:
-- 
2.29.2

[PATCH v2 26/33] iotests/222: fix pylint and mypy complains

[Vladimir Sementsov-Ogievskiy]

Here:
 - long line
 - move to new interface of vm.qmp() (direct passing dict), to avoid
   mypy false-positive, as it thinks that unpacked dict is a positional
   argument.
 - extra parenthesis
 - handle event_wait possible None value

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/222 | 20 +++-
 tests/qemu-iotests/297 |  2 +-
 2 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/tests/qemu-iotests/222 b/tests/qemu-iotests/222
index b48afe623e..5e2556f8df 100755
--- a/tests/qemu-iotests/222
+++ b/tests/qemu-iotests/222
@@ -50,7 +50,8 @@ remainder = [("0xd5", "0x108000",  "32k"), # Right-end of 
partial-left [1]
 
 with iotests.FilePath('base.img') as base_img_path, \
  iotests.FilePath('fleece.img') as fleece_img_path, \
- iotests.FilePath('nbd.sock', base_dir=iotests.sock_dir) as nbd_sock_path, 
\
+ iotests.FilePath('nbd.sock',
+  base_dir=iotests.sock_dir) as nbd_sock_path, \
  iotests.VM() as vm:
 
 log('--- Setting up images ---')
@@ -81,7 +82,7 @@ with iotests.FilePath('base.img') as base_img_path, \
 tgt_node = "fleeceNode"
 
 # create tgt_node backed by src_node
-log(vm.qmp("blockdev-add", **{
+log(vm.qmp("blockdev-add", {
 "driver": "qcow2",
 "node-name": tgt_node,
 "file": {
@@ -103,8 +104,8 @@ with iotests.FilePath('base.img') as base_img_path, \
 
 nbd_uri = 'nbd+unix:///%s?socket=%s' % (tgt_node, nbd_sock_path)
 log(vm.qmp("nbd-server-start",
-   **{"addr": { "type": "unix",
-"data": { "path": nbd_sock_path } } }))
+   {"addr": { "type": "unix",
+  "data": { "path": nbd_sock_path } } }))
 
 log(vm.qmp("nbd-server-add", device=tgt_node))
 
@@ -112,7 +113,7 @@ with iotests.FilePath('base.img') as base_img_path, \
 log('--- Sanity Check ---')
 log('')
 
-for p in (patterns + zeroes):
+for p in patterns + zeroes:
 cmd = "read -P%s %s %s" % p
 log(cmd)
 assert qemu_io_silent('-r', '-f', 'raw', '-c', cmd, nbd_uri) == 0
@@ -130,7 +131,7 @@ with iotests.FilePath('base.img') as base_img_path, \
 log('--- Verifying Data ---')
 log('')
 
-for p in (patterns + zeroes):
+for p in patterns + zeroes:
 cmd = "read -P%s %s %s" % p
 log(cmd)
 assert qemu_io_silent('-r', '-f', 'raw', '-c', cmd, nbd_uri) == 0
@@ -140,8 +141,9 @@ with iotests.FilePath('base.img') as base_img_path, \
 log('')
 
 log(vm.qmp('block-job-cancel', device=src_node))
-log(vm.event_wait('BLOCK_JOB_CANCELLED'),
-filters=[iotests.filter_qmp_event])
+e = vm.event_wait('BLOCK_JOB_CANCELLED')
+assert e is not None
+log(e, filters=[iotests.filter_qmp_event])
 log(vm.qmp('nbd-server-stop'))
 log(vm.qmp('blockdev-del', node_name=tgt_node))
 vm.shutdown()
@@ -150,7 +152,7 @@ with iotests.FilePath('base.img') as base_img_path, \
 log('--- Confirming writes ---')
 log('')
 
-for p in (overwrite + remainder):
+for p in overwrite + remainder:
 cmd = "read -P%s %s %s" % p
 log(cmd)
 assert qemu_io_silent(base_img_path, '-c', cmd) == 0
diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index a37910b42d..7cb8c531fd 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -31,7 +31,7 @@ SKIP_FILES = (
 '096', '118', '124', '132', '136', '139', '147', '148', '149',
 '151', '152', '155', '163', '165', '169', '194', '196', '199', '202',
 '203', '205', '206', '207', '208', '210', '211', '212', '213', '216',
-'218', '219', '222', '224', '228', '234', '235', '236', '237', '238',
+'218', '219', '224', '228', '234', '235', '236', '237', '238',
 '240', '242', '245', '246', '248', '255', '256', '257', '258', '260',
 '262', '264', '266', '274', '277', '280', '281', '295', '296', '298',
 '299', '302', '303', '304', '307',
-- 
2.29.2

[PATCH v2 25/33] iotests.py: VM: add own __enter__ method

[Vladimir Sementsov-Ogievskiy]

In superclass __enter__ method is defined with return value type hint
'QEMUMachine'. So, mypy thinks that return value of VM.__enter__ is
QEMUMachine. Let's redefine __enter__ in VM class, to give it correct
type hint.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/iotests.py | 4 
 1 file changed, 4 insertions(+)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 777fa2ec0e..c7ec16c082 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -568,6 +568,10 @@ def remote_filename(path):
 class VM(qtest.QEMUQtestMachine):
 '''A QEMU VM'''
 
+# Redefine __enter__ with proper type hint
+def __enter__(self) -> 'VM':
+return self
+
 def __init__(self, path_suffix=''):
 name = "qemu%s-%d" % (path_suffix, os.getpid())
 super().__init__(qemu_prog, qemu_opts, name=name,
-- 
2.29.2

[PATCH v2 11/33] block/copy-before-write: relax permission requirements when no parents

[Vladimir Sementsov-Ogievskiy]

We are going to publish copy-before-write filter. So, user should be
able to create it with blockdev-add first, specifying both filtered and
target children. And then do blockdev-reopen, to actually insert the
filter where needed.

Currently, filter unshares write permission unconditionally on source
node. It's good, but it will not allow to do blockdev-add. So, let's
relax restrictions when filter doesn't have any parent.

Test output is modified, as now permission conflict happens only when
job creates a blk parent for filter node.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
---
 block/copy-before-write.c  | 8 +---
 tests/qemu-iotests/283.out | 2 +-
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/block/copy-before-write.c b/block/copy-before-write.c
index a7996d54db..2a51cc64e4 100644
--- a/block/copy-before-write.c
+++ b/block/copy-before-write.c
@@ -142,10 +142,12 @@ static void cbw_child_perm(BlockDriverState *bs, 
BdrvChild *c,
 bdrv_default_perms(bs, c, role, reopen_queue,
perm, shared, nperm, nshared);
 
-if (perm & BLK_PERM_WRITE) {
-*nperm = *nperm | BLK_PERM_CONSISTENT_READ;
+if (!QLIST_EMPTY(&bs->parents)) {
+if (perm & BLK_PERM_WRITE) {
+*nperm = *nperm | BLK_PERM_CONSISTENT_READ;
+}
+*nshared &= ~(BLK_PERM_WRITE | BLK_PERM_RESIZE);
 }
-*nshared &= ~(BLK_PERM_WRITE | BLK_PERM_RESIZE);
 }
 }
 
diff --git a/tests/qemu-iotests/283.out b/tests/qemu-iotests/283.out
index e08f807dab..d5350ce7a7 100644
--- a/tests/qemu-iotests/283.out
+++ b/tests/qemu-iotests/283.out
@@ -5,7 +5,7 @@
 {"execute": "blockdev-add", "arguments": {"driver": "blkdebug", "image": 
"base", "node-name": "other", "take-child-perms": ["write"]}}
 {"return": {}}
 {"execute": "blockdev-backup", "arguments": {"device": "source", "sync": 
"full", "target": "target"}}
-{"error": {"class": "GenericError", "desc": "Cannot append copy-before-write 
filter: Conflicts with use by source as 'image', which does not allow 'write' 
on base"}}
+{"error": {"class": "GenericError", "desc": "Conflicts with use by source as 
'image', which does not allow 'write' on base"}}
 
 === copy-before-write filter should be gone after job-finalize ===
 
-- 
2.29.2

[PATCH v2 16/33] block/copy-before-write: cbw_init(): rename variables

[Vladimir Sementsov-Ogievskiy]

One more step closer to real .bdrv_open() handler: use more usual names
for bs being initialized and its state.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
---
 block/copy-before-write.c | 29 ++---
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/block/copy-before-write.c b/block/copy-before-write.c
index a4fee645fd..d7f1833efa 100644
--- a/block/copy-before-write.c
+++ b/block/copy-before-write.c
@@ -144,38 +144,37 @@ static void cbw_child_perm(BlockDriverState *bs, 
BdrvChild *c,
 }
 }
 
-static int cbw_init(BlockDriverState *top, BlockDriverState *source,
+static int cbw_init(BlockDriverState *bs, BlockDriverState *source,
 BlockDriverState *target, bool compress, Error **errp)
 {
-BDRVCopyBeforeWriteState *state = top->opaque;
+BDRVCopyBeforeWriteState *s = bs->opaque;
 
-top->total_sectors = source->total_sectors;
-top->supported_write_flags = BDRV_REQ_WRITE_UNCHANGED |
+bs->total_sectors = source->total_sectors;
+bs->supported_write_flags = BDRV_REQ_WRITE_UNCHANGED |
 (BDRV_REQ_FUA & source->supported_write_flags);
-top->supported_zero_flags = BDRV_REQ_WRITE_UNCHANGED |
+bs->supported_zero_flags = BDRV_REQ_WRITE_UNCHANGED |
 ((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) &
  source->supported_zero_flags);
 
 bdrv_ref(target);
-state->target = bdrv_attach_child(top, target, "target", &child_of_bds,
-  BDRV_CHILD_DATA, errp);
-if (!state->target) {
+s->target = bdrv_attach_child(bs, target, "target", &child_of_bds,
+  BDRV_CHILD_DATA, errp);
+if (!s->target) {
 error_prepend(errp, "Cannot attach target child: ");
 return -EINVAL;
 }
 
 bdrv_ref(source);
-top->file = bdrv_attach_child(top, source, "file", &child_of_bds,
-  BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
-  errp);
-if (!top->file) {
+bs->file = bdrv_attach_child(bs, source, "file", &child_of_bds,
+ BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
+ errp);
+if (!bs->file) {
 error_prepend(errp, "Cannot attach file child: ");
 return -EINVAL;
 }
 
-state->bcs = block_copy_state_new(top->file, state->target, false, 
compress,
-  errp);
-if (!state->bcs) {
+s->bcs = block_copy_state_new(bs->file, s->target, false, compress, errp);
+if (!s->bcs) {
 error_prepend(errp, "Cannot create block-copy-state: ");
 return -EINVAL;
 }
-- 
2.29.2

[PATCH v2 19/33] block/copy-before-write: initialize block-copy bitmap

[Vladimir Sementsov-Ogievskiy]

We are going to publish copy-before-write filter to be used in separate
of backup. Future step would support bitmap for the filter. But let's
start from full set bitmap.

We have to modify backup, as bitmap is first initialized by
copy-before-write filter, and then backup modifies it.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/backup.c| 16 +++-
 block/copy-before-write.c |  4 
 2 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index af17149f22..14652ac98a 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -233,18 +233,16 @@ static void backup_init_bcs_bitmap(BackupBlockJob *job)
 BdrvDirtyBitmap *bcs_bitmap = block_copy_dirty_bitmap(job->bcs);
 
 if (job->sync_mode == MIRROR_SYNC_MODE_BITMAP) {
+bdrv_clear_dirty_bitmap(bcs_bitmap, NULL);
 ret = bdrv_dirty_bitmap_merge_internal(bcs_bitmap, job->sync_bitmap,
NULL, true);
 assert(ret);
-} else {
-if (job->sync_mode == MIRROR_SYNC_MODE_TOP) {
-/*
- * We can't hog the coroutine to initialize this thoroughly.
- * Set a flag and resume work when we are able to yield safely.
- */
-block_copy_set_skip_unallocated(job->bcs, true);
-}
-bdrv_set_dirty_bitmap(bcs_bitmap, 0, job->len);
+} else if (job->sync_mode == MIRROR_SYNC_MODE_TOP) {
+/*
+ * We can't hog the coroutine to initialize this thoroughly.
+ * Set a flag and resume work when we are able to yield safely.
+ */
+block_copy_set_skip_unallocated(job->bcs, true);
 }
 
 estimate = bdrv_get_dirty_count(bcs_bitmap);
diff --git a/block/copy-before-write.c b/block/copy-before-write.c
index e889af7b80..ba2466a328 100644
--- a/block/copy-before-write.c
+++ b/block/copy-before-write.c
@@ -147,6 +147,7 @@ static void cbw_child_perm(BlockDriverState *bs, BdrvChild 
*c,
 static int cbw_init(BlockDriverState *bs, QDict *options, Error **errp)
 {
 BDRVCopyBeforeWriteState *s = bs->opaque;
+BdrvDirtyBitmap *copy_bitmap;
 bool compress;
 
 bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds,
@@ -184,6 +185,9 @@ static int cbw_init(BlockDriverState *bs, QDict *options, 
Error **errp)
 return -EINVAL;
 }
 
+copy_bitmap = block_copy_dirty_bitmap(s->bcs);
+bdrv_set_dirty_bitmap(copy_bitmap, 0, bdrv_dirty_bitmap_size(copy_bitmap));
+
 return 0;
 }
 
-- 
2.29.2

[PATCH v2 18/33] block/copy-before-write: cbw_init(): use options

[Vladimir Sementsov-Ogievskiy]

One more step closer to .bdrv_open(): use options instead of plain
arguments. Move to bdrv_open_child() calls, native for drive open
handlers.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/copy-before-write.c | 40 +--
 1 file changed, 26 insertions(+), 14 deletions(-)

diff --git a/block/copy-before-write.c b/block/copy-before-write.c
index 4858dcf8ff..e889af7b80 100644
--- a/block/copy-before-write.c
+++ b/block/copy-before-write.c
@@ -144,25 +144,21 @@ static void cbw_child_perm(BlockDriverState *bs, 
BdrvChild *c,
 }
 }
 
-static int cbw_init(BlockDriverState *bs, BlockDriverState *source,
-BlockDriverState *target, bool compress, Error **errp)
+static int cbw_init(BlockDriverState *bs, QDict *options, Error **errp)
 {
 BDRVCopyBeforeWriteState *s = bs->opaque;
+bool compress;
 
-bdrv_ref(target);
-s->target = bdrv_attach_child(bs, target, "target", &child_of_bds,
-  BDRV_CHILD_DATA, errp);
-if (!s->target) {
-error_prepend(errp, "Cannot attach target child: ");
+bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds,
+   BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
+   false, errp);
+if (!bs->file) {
 return -EINVAL;
 }
 
-bdrv_ref(source);
-bs->file = bdrv_attach_child(bs, source, "file", &child_of_bds,
- BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
- errp);
-if (!bs->file) {
-error_prepend(errp, "Cannot attach file child: ");
+s->target = bdrv_open_child(NULL, options, "target", bs, &child_of_bds,
+BDRV_CHILD_DATA, false, errp);
+if (!s->target) {
 return -EINVAL;
 }
 
@@ -173,6 +169,15 @@ static int cbw_init(BlockDriverState *bs, BlockDriverState 
*source,
 ((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) &
  bs->file->bs->supported_zero_flags);
 
+/*
+ * The option x-deprecated-compress is only for internal use, to support
+ * compress option of backup job. New users should instead use compress
+ * filter, if they want output to be compressed.
+ * Moreover compress option of backup should probably be deprecated too.
+ * So, x-deprecated-compress is not visible through QMP.
+ */
+compress = qdict_get_try_bool(options, "x-deprecated-compress", false);
+qdict_del(options, "x-deprecated-compress");
 s->bcs = block_copy_state_new(bs->file, s->target, false, compress, errp);
 if (!s->bcs) {
 error_prepend(errp, "Cannot create block-copy-state: ");
@@ -210,6 +215,7 @@ BlockDriverState *bdrv_cbw_append(BlockDriverState *source,
 int ret;
 BDRVCopyBeforeWriteState *state;
 BlockDriverState *top;
+QDict *opts;
 
 assert(source->total_sectors == target->total_sectors);
 
@@ -221,7 +227,13 @@ BlockDriverState *bdrv_cbw_append(BlockDriverState *source,
 }
 state = top->opaque;
 
-ret = cbw_init(top, source, target, compress, errp);
+opts = qdict_new();
+qdict_put_str(opts, "file", bdrv_get_node_name(source));
+qdict_put_str(opts, "target", bdrv_get_node_name(target));
+qdict_put_bool(opts, "x-deprecated-compress", compress);
+
+ret = cbw_init(top, opts, errp);
+qobject_unref(opts);
 if (ret < 0) {
 goto fail;
 }
-- 
2.29.2

[PATCH v2 06/33] qdev: allow setting drive property for realized device

[Vladimir Sementsov-Ogievskiy]

We need an ability to insert filters above top block node, attached to
block device. It can't be achieved with blockdev-reopen command. So, we
want do it with help of qom-set.

Intended usage:

Assume there is a node A that is attached to some guest device.

1. blockdev-add to create a filter node B that has A as its child.

2. qom-set to change the node attached to the guest device’s
   BlockBackend from A to B.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 hw/core/qdev-properties-system.c | 43 +++-
 1 file changed, 31 insertions(+), 12 deletions(-)

diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index 2760c21f11..e71f5d64d1 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -36,11 +36,11 @@
 
 static bool check_prop_still_unset(Object *obj, const char *name,
const void *old_val, const char *new_val,
-   Error **errp)
+   bool allow_override, Error **errp)
 {
 const GlobalProperty *prop = qdev_find_global_prop(obj, name);
 
-if (!old_val) {
+if (!old_val || (!prop && allow_override)) {
 return true;
 }
 
@@ -93,16 +93,34 @@ static void set_drive_helper(Object *obj, Visitor *v, const 
char *name,
 BlockBackend *blk;
 bool blk_created = false;
 int ret;
+BlockDriverState *bs;
+AioContext *ctx;
 
 if (!visit_type_str(v, name, &str, errp)) {
 return;
 }
 
-/*
- * TODO Should this really be an error?  If no, the old value
- * needs to be released before we store the new one.
- */
-if (!check_prop_still_unset(obj, name, *ptr, str, errp)) {
+if (!check_prop_still_unset(obj, name, *ptr, str, true, errp)) {
+return;
+}
+
+if (*ptr) {
+/* BlockBackend alread exists. So, we want to change attached node */
+blk = *ptr;
+ctx = blk_get_aio_context(blk);
+bs = bdrv_lookup_bs(NULL, str, errp);
+if (!bs) {
+return;
+}
+
+if (ctx != bdrv_get_aio_context(bs)) {
+error_setg(errp, "Different aio context is not supported for new "
+   "node");
+}
+
+aio_context_acquire(ctx);
+blk_replace_bs(blk, bs, errp);
+aio_context_release(ctx);
 return;
 }
 
@@ -114,7 +132,7 @@ static void set_drive_helper(Object *obj, Visitor *v, const 
char *name,
 
 blk = blk_by_name(str);
 if (!blk) {
-BlockDriverState *bs = bdrv_lookup_bs(NULL, str, NULL);
+bs = bdrv_lookup_bs(NULL, str, NULL);
 if (bs) {
 /*
  * If the device supports iothreads, it will make sure to move the
@@ -123,8 +141,7 @@ static void set_drive_helper(Object *obj, Visitor *v, const 
char *name,
  * aware of iothreads require their BlockBackends to be in the main
  * AioContext.
  */
-AioContext *ctx = iothread ? bdrv_get_aio_context(bs) :
- qemu_get_aio_context();
+ctx = iothread ? bdrv_get_aio_context(bs) : qemu_get_aio_context();
 blk = blk_new(ctx, 0, BLK_PERM_ALL);
 blk_created = true;
 
@@ -196,6 +213,7 @@ static void release_drive(Object *obj, const char *name, 
void *opaque)
 const PropertyInfo qdev_prop_drive = {
 .name  = "str",
 .description = "Node name or ID of a block device to use as a backend",
+.realized_set_allowed = true,
 .get   = get_drive,
 .set   = set_drive,
 .release = release_drive,
@@ -204,6 +222,7 @@ const PropertyInfo qdev_prop_drive = {
 const PropertyInfo qdev_prop_drive_iothread = {
 .name  = "str",
 .description = "Node name or ID of a block device to use as a backend",
+.realized_set_allowed = true,
 .get   = get_drive,
 .set   = set_drive_iothread,
 .release = release_drive,
@@ -238,7 +257,7 @@ static void set_chr(Object *obj, Visitor *v, const char 
*name, void *opaque,
  * TODO Should this really be an error?  If no, the old value
  * needs to be released before we store the new one.
  */
-if (!check_prop_still_unset(obj, name, be->chr, str, errp)) {
+if (!check_prop_still_unset(obj, name, be->chr, str, false, errp)) {
 return;
 }
 
@@ -408,7 +427,7 @@ static void set_netdev(Object *obj, Visitor *v, const char 
*name,
  * TODO Should this really be an error?  If no, the old value
  * needs to be released before we store the new one.
  */
-if (!check_prop_still_unset(obj, name, ncs[i], str, errp)) {
+if (!check_prop_still_unset(obj, name, ncs[i], str, false, errp)) {
 goto out;
 }
 
-- 
2.29.2

[PATCH v2 15/33] block/copy-before-write: introduce cbw_init()

[Vladimir Sementsov-Ogievskiy]

Move part of bdrv_cbw_append() to new function cbw_open(). It's an
intermediate step for adding noramal .bdrv_open() handler to the
filter. With this commit no logic is changed, but we have a function
which will be turned into .bdrv_open() handler in future commit.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
---
 block/copy-before-write.c | 69 +++
 1 file changed, 41 insertions(+), 28 deletions(-)

diff --git a/block/copy-before-write.c b/block/copy-before-write.c
index adbbc64aa9..a4fee645fd 100644
--- a/block/copy-before-write.c
+++ b/block/copy-before-write.c
@@ -144,6 +144,45 @@ static void cbw_child_perm(BlockDriverState *bs, BdrvChild 
*c,
 }
 }
 
+static int cbw_init(BlockDriverState *top, BlockDriverState *source,
+BlockDriverState *target, bool compress, Error **errp)
+{
+BDRVCopyBeforeWriteState *state = top->opaque;
+
+top->total_sectors = source->total_sectors;
+top->supported_write_flags = BDRV_REQ_WRITE_UNCHANGED |
+(BDRV_REQ_FUA & source->supported_write_flags);
+top->supported_zero_flags = BDRV_REQ_WRITE_UNCHANGED |
+((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) &
+ source->supported_zero_flags);
+
+bdrv_ref(target);
+state->target = bdrv_attach_child(top, target, "target", &child_of_bds,
+  BDRV_CHILD_DATA, errp);
+if (!state->target) {
+error_prepend(errp, "Cannot attach target child: ");
+return -EINVAL;
+}
+
+bdrv_ref(source);
+top->file = bdrv_attach_child(top, source, "file", &child_of_bds,
+  BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
+  errp);
+if (!top->file) {
+error_prepend(errp, "Cannot attach file child: ");
+return -EINVAL;
+}
+
+state->bcs = block_copy_state_new(top->file, state->target, false, 
compress,
+  errp);
+if (!state->bcs) {
+error_prepend(errp, "Cannot create block-copy-state: ");
+return -EINVAL;
+}
+
+return 0;
+}
+
 BlockDriver bdrv_cbw_filter = {
 .format_name = "copy-before-write",
 .instance_size = sizeof(BDRVCopyBeforeWriteState),
@@ -181,36 +220,10 @@ BlockDriverState *bdrv_cbw_append(BlockDriverState 
*source,
 error_prepend(errp, "Cannot open driver: ");
 return NULL;
 }
-
 state = top->opaque;
-top->total_sectors = source->total_sectors;
-top->supported_write_flags = BDRV_REQ_WRITE_UNCHANGED |
-(BDRV_REQ_FUA & source->supported_write_flags);
-top->supported_zero_flags = BDRV_REQ_WRITE_UNCHANGED |
-((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) &
- source->supported_zero_flags);
-
-bdrv_ref(target);
-state->target = bdrv_attach_child(top, target, "target", &child_of_bds,
-  BDRV_CHILD_DATA, errp);
-if (!state->target) {
-error_prepend(errp, "Cannot attach target child: ");
-goto fail;
-}
 
-bdrv_ref(source);
-top->file = bdrv_attach_child(top, source, "file", &child_of_bds,
-  BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
-  errp);
-if (!top->file) {
-error_prepend(errp, "Cannot attach file child: ");
-goto fail;
-}
-
-state->bcs = block_copy_state_new(top->file, state->target, false, 
compress,
-  errp);
-if (!state->bcs) {
-error_prepend(errp, "Cannot create block-copy-state: ");
+ret = cbw_init(top, source, target, compress, errp);
+if (ret < 0) {
 goto fail;
 }
 
-- 
2.29.2

[PATCH v2 08/33] block/backup: drop support for copy_range

[Vladimir Sementsov-Ogievskiy]

copy_range is not a default behavior since 6a30f663d4c0b3c, and it's
now available only though x-perf experimantal argument, so it's OK to
drop it.

Even when backup is used to copy disk to same filesystem, and
filesystem supports zero-copy copy_range, copy_range is probably not
what we want for backup: backup has good property of making a copy of
active disk, with no impact to active disk itself (unlike creating a
snapshot). And if copy_range instead of copying data adds fs-level
references, and on next guest write COW operation occurs, it's seems
most possible, that new block will be allocated for original vm disk,
not for backup disk. Thus, fragmentation of original disk will
increase.

We can simply add support back on demand. Now we want to publish
copy-before-write filter, and instead of thinking how to pass
use-copy-range argument to block-copy (create x-block-copy parameter
for new public filter driver, or may be set it by hand after filter
node creation?), instead of this let's just drop copy-range support in
backup for now.

After this patch copy-range support in block-copy becomes unused. Let's
keep it for a while, it won't hurt:

1. If there would be request for supporting copy_range in backup
   (and/or in a new public copy-before-write filter), it will be easy
   to satisfy it.

2. Probably, qemu-img convert will reuse block-copy, and qemu-img has
   option to enable copy-range. qemu-img convert is not a backup, and
   copy_range may be more reasonable for some cases in context of
   qemu-img convert.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
---
 block/copy-before-write.h | 1 -
 block/backup.c| 3 +--
 block/copy-before-write.c | 4 +---
 3 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/block/copy-before-write.h b/block/copy-before-write.h
index 5977b7aa31..e284dfb6a7 100644
--- a/block/copy-before-write.h
+++ b/block/copy-before-write.h
@@ -33,7 +33,6 @@ BlockDriverState *bdrv_cbw_append(BlockDriverState *source,
   BlockDriverState *target,
   const char *filter_node_name,
   uint64_t cluster_size,
-  BackupPerf *perf,
   BdrvRequestFlags write_flags,
   BlockCopyState **bcs,
   Error **errp);
diff --git a/block/backup.c b/block/backup.c
index ac91821b08..d41dd30e25 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -522,8 +522,7 @@ BlockJob *backup_job_create(const char *job_id, 
BlockDriverState *bs,
   (compress ? BDRV_REQ_WRITE_COMPRESSED : 0),
 
 cbw = bdrv_cbw_append(bs, target, filter_node_name,
-cluster_size, perf,
-write_flags, &bcs, errp);
+  cluster_size, write_flags, &bcs, errp);
 if (!cbw) {
 goto error;
 }
diff --git a/block/copy-before-write.c b/block/copy-before-write.c
index 0dc5a107cf..bc795adb87 100644
--- a/block/copy-before-write.c
+++ b/block/copy-before-write.c
@@ -170,7 +170,6 @@ BlockDriverState *bdrv_cbw_append(BlockDriverState *source,
   BlockDriverState *target,
   const char *filter_node_name,
   uint64_t cluster_size,
-  BackupPerf *perf,
   BdrvRequestFlags write_flags,
   BlockCopyState **bcs,
   Error **errp)
@@ -217,8 +216,7 @@ BlockDriverState *bdrv_cbw_append(BlockDriverState *source,
 
 state->cluster_size = cluster_size;
 state->bcs = block_copy_state_new(top->backing, state->target,
-  cluster_size, perf->use_copy_range,
-  write_flags, errp);
+  cluster_size, false, write_flags, errp);
 if (!state->bcs) {
 error_prepend(errp, "Cannot create block-copy-state: ");
 goto fail;
-- 
2.29.2

[PATCH v2 14/33] block/copy-before-write: bdrv_cbw_append(): replace child at last

[Vladimir Sementsov-Ogievskiy]

Refactor the function to replace child at last. Thus we don't need to
revert it and code is simplified.

block-copy state initilization being done before replacing the child
doesn't need any drained section.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
---
 block/copy-before-write.c | 33 +++--
 1 file changed, 11 insertions(+), 22 deletions(-)

diff --git a/block/copy-before-write.c b/block/copy-before-write.c
index 7a6c15f141..adbbc64aa9 100644
--- a/block/copy-before-write.c
+++ b/block/copy-before-write.c
@@ -172,7 +172,6 @@ BlockDriverState *bdrv_cbw_append(BlockDriverState *source,
 int ret;
 BDRVCopyBeforeWriteState *state;
 BlockDriverState *top;
-bool appended = false;
 
 assert(source->total_sectors == target->total_sectors);
 
@@ -196,8 +195,7 @@ BlockDriverState *bdrv_cbw_append(BlockDriverState *source,
   BDRV_CHILD_DATA, errp);
 if (!state->target) {
 error_prepend(errp, "Cannot attach target child: ");
-bdrv_unref(top);
-return NULL;
+goto fail;
 }
 
 bdrv_ref(source);
@@ -206,18 +204,8 @@ BlockDriverState *bdrv_cbw_append(BlockDriverState *source,
   errp);
 if (!top->file) {
 error_prepend(errp, "Cannot attach file child: ");
-bdrv_unref(top);
-return NULL;
-}
-
-bdrv_drained_begin(source);
-
-ret = bdrv_replace_node(source, top, errp);
-if (ret < 0) {
-error_prepend(errp, "Cannot append copy-before-write filter: ");
 goto fail;
 }
-appended = true;
 
 state->bcs = block_copy_state_new(top->file, state->target, false, 
compress,
   errp);
@@ -225,21 +213,22 @@ BlockDriverState *bdrv_cbw_append(BlockDriverState 
*source,
 error_prepend(errp, "Cannot create block-copy-state: ");
 goto fail;
 }
-*bcs = state->bcs;
 
+bdrv_drained_begin(source);
+ret = bdrv_replace_node(source, top, errp);
 bdrv_drained_end(source);
+if (ret < 0) {
+error_prepend(errp, "Cannot append copy-before-write filter: ");
+goto fail;
+}
+
+*bcs = state->bcs;
 
 return top;
 
 fail:
-if (appended) {
-bdrv_cbw_drop(top);
-} else {
-bdrv_unref(top);
-}
-
-bdrv_drained_end(source);
-
+block_copy_state_free(state->bcs);
+bdrv_unref(top);
 return NULL;
 }
 
-- 
2.29.2

[PATCH v2 12/33] block/copy-before-write: drop extra bdrv_unref on failure path

[Vladimir Sementsov-Ogievskiy]

bdrv_attach_child() do bdrv_unref() on failure, so we shouldn't do it
by hand here.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/copy-before-write.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/block/copy-before-write.c b/block/copy-before-write.c
index 2a51cc64e4..945d9340f4 100644
--- a/block/copy-before-write.c
+++ b/block/copy-before-write.c
@@ -201,7 +201,6 @@ BlockDriverState *bdrv_cbw_append(BlockDriverState *source,
 state->target = bdrv_attach_child(top, target, "target", &child_of_bds,
   BDRV_CHILD_DATA, errp);
 if (!state->target) {
-bdrv_unref(target);
 bdrv_unref(top);
 return NULL;
 }
-- 
2.29.2

[PATCH v2 28/33] iotests: move 222 to tests/image-fleecing

[Vladimir Sementsov-Ogievskiy]

Give a good name to test file.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/{222 => tests/image-fleecing} | 0
 tests/qemu-iotests/{222.out => tests/image-fleecing.out} | 0
 2 files changed, 0 insertions(+), 0 deletions(-)
 rename tests/qemu-iotests/{222 => tests/image-fleecing} (100%)
 rename tests/qemu-iotests/{222.out => tests/image-fleecing.out} (100%)

diff --git a/tests/qemu-iotests/222 b/tests/qemu-iotests/tests/image-fleecing
similarity index 100%
rename from tests/qemu-iotests/222
rename to tests/qemu-iotests/tests/image-fleecing
diff --git a/tests/qemu-iotests/222.out 
b/tests/qemu-iotests/tests/image-fleecing.out
similarity index 100%
rename from tests/qemu-iotests/222.out
rename to tests/qemu-iotests/tests/image-fleecing.out
-- 
2.29.2

[PATCH v2 23/33] python/qemu/machine.py: refactor _qemu_args()

[Vladimir Sementsov-Ogievskiy]

 - use shorter construction
 - don't create new dict if not needed
 - drop extra unpacking key-val arguments
 - drop extra default values

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 python/qemu/machine.py | 18 --
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index 6e44bda337..ff35f2cd6c 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -541,14 +541,12 @@ def _qmp(self) -> qmp.QEMUMonitorProtocol:
 return self._qmp_connection
 
 @classmethod
-def _qmp_args(cls, _conv_keys: bool = True, **args: Any) -> Dict[str, Any]:
-qmp_args = dict()
-for key, value in args.items():
-if _conv_keys:
-qmp_args[key.replace('_', '-')] = value
-else:
-qmp_args[key] = value
-return qmp_args
+def _qmp_args(cls, conv_keys: bool,
+  args: Dict[str, Any]) -> Dict[str, Any]:
+if conv_keys:
+return {k.replace('_', '-'): v for k, v in args.items()}
+else:
+return args
 
 def qmp(self, cmd: str,
 conv_keys: bool = True,
@@ -556,7 +554,7 @@ def qmp(self, cmd: str,
 """
 Invoke a QMP command and return the response dict
 """
-qmp_args = self._qmp_args(conv_keys, **args)
+qmp_args = self._qmp_args(conv_keys, args)
 return self._qmp.cmd(cmd, args=qmp_args)
 
 def command(self, cmd: str,
@@ -567,7 +565,7 @@ def command(self, cmd: str,
 On success return the response dict.
 On failure raise an exception.
 """
-qmp_args = self._qmp_args(conv_keys, **args)
+qmp_args = self._qmp_args(conv_keys, args)
 return self._qmp.command(cmd, **qmp_args)
 
 def get_qmp_event(self, wait: bool = False) -> Optional[QMPMessage]:
-- 
2.29.2

[PATCH v2 13/33] block/copy-before-write: use file child instead of backing

[Vladimir Sementsov-Ogievskiy]

We are going to publish copy-before-write filter, and there no public
backing-child-based filter in Qemu. No reason to create a precedent, so
let's refactor copy-before-write filter instead.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/copy-before-write.c | 39 ++-
 1 file changed, 22 insertions(+), 17 deletions(-)

diff --git a/block/copy-before-write.c b/block/copy-before-write.c
index 945d9340f4..7a6c15f141 100644
--- a/block/copy-before-write.c
+++ b/block/copy-before-write.c
@@ -43,7 +43,7 @@ static coroutine_fn int cbw_co_preadv(
 BlockDriverState *bs, uint64_t offset, uint64_t bytes,
 QEMUIOVector *qiov, int flags)
 {
-return bdrv_co_preadv(bs->backing, offset, bytes, qiov, flags);
+return bdrv_co_preadv(bs->file, offset, bytes, qiov, flags);
 }
 
 static coroutine_fn int cbw_do_copy_before_write(BlockDriverState *bs,
@@ -71,7 +71,7 @@ static int coroutine_fn cbw_co_pdiscard(BlockDriverState *bs,
 return ret;
 }
 
-return bdrv_co_pdiscard(bs->backing, offset, bytes);
+return bdrv_co_pdiscard(bs->file, offset, bytes);
 }
 
 static int coroutine_fn cbw_co_pwrite_zeroes(BlockDriverState *bs,
@@ -82,7 +82,7 @@ static int coroutine_fn cbw_co_pwrite_zeroes(BlockDriverState 
*bs,
 return ret;
 }
 
-return bdrv_co_pwrite_zeroes(bs->backing, offset, bytes, flags);
+return bdrv_co_pwrite_zeroes(bs->file, offset, bytes, flags);
 }
 
 static coroutine_fn int cbw_co_pwritev(BlockDriverState *bs,
@@ -95,29 +95,22 @@ static coroutine_fn int cbw_co_pwritev(BlockDriverState *bs,
 return ret;
 }
 
-return bdrv_co_pwritev(bs->backing, offset, bytes, qiov, flags);
+return bdrv_co_pwritev(bs->file, offset, bytes, qiov, flags);
 }
 
 static int coroutine_fn cbw_co_flush(BlockDriverState *bs)
 {
-if (!bs->backing) {
+if (!bs->file) {
 return 0;
 }
 
-return bdrv_co_flush(bs->backing->bs);
+return bdrv_co_flush(bs->file->bs);
 }
 
 static void cbw_refresh_filename(BlockDriverState *bs)
 {
-if (bs->backing == NULL) {
-/*
- * we can be here after failed bdrv_attach_child in
- * bdrv_set_backing_hd
- */
-return;
-}
 pstrcpy(bs->exact_filename, sizeof(bs->exact_filename),
-bs->backing->bs->filename);
+bs->file->bs->filename);
 }
 
 static void cbw_child_perm(BlockDriverState *bs, BdrvChild *c,
@@ -186,6 +179,7 @@ BlockDriverState *bdrv_cbw_append(BlockDriverState *source,
 top = bdrv_new_open_driver(&bdrv_cbw_filter, filter_node_name,
BDRV_O_RDWR, errp);
 if (!top) {
+error_prepend(errp, "Cannot open driver: ");
 return NULL;
 }
 
@@ -201,21 +195,32 @@ BlockDriverState *bdrv_cbw_append(BlockDriverState 
*source,
 state->target = bdrv_attach_child(top, target, "target", &child_of_bds,
   BDRV_CHILD_DATA, errp);
 if (!state->target) {
+error_prepend(errp, "Cannot attach target child: ");
+bdrv_unref(top);
+return NULL;
+}
+
+bdrv_ref(source);
+top->file = bdrv_attach_child(top, source, "file", &child_of_bds,
+  BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
+  errp);
+if (!top->file) {
+error_prepend(errp, "Cannot attach file child: ");
 bdrv_unref(top);
 return NULL;
 }
 
 bdrv_drained_begin(source);
 
-ret = bdrv_append(top, source, errp);
+ret = bdrv_replace_node(source, top, errp);
 if (ret < 0) {
 error_prepend(errp, "Cannot append copy-before-write filter: ");
 goto fail;
 }
 appended = true;
 
-state->bcs = block_copy_state_new(top->backing, state->target,
-  false, compress, errp);
+state->bcs = block_copy_state_new(top->file, state->target, false, 
compress,
+  errp);
 if (!state->bcs) {
 error_prepend(errp, "Cannot create block-copy-state: ");
 goto fail;
-- 
2.29.2

[PATCH v2 07/33] block: rename backup-top to copy-before-write

[Vladimir Sementsov-Ogievskiy]

We are going to convert backup_top to full featured public filter,
which can be used in separate of backup job. Start from renaming from
"how it used" to "what it does".

While updating comments in 283 iotest, drop and rephrase also things
about ".active", as this field is now dropped, and filter doesn't have
"inactive" mode.

Note that this change may be considered as incompatible interface
change, as backup-top filter format name was visible through
query-block and query-named-block-nodes.

Still, consider the following reasoning:

1. backup-top was never documented, so if someone depends on format
   name (for driver that can't be used other than it is automatically
   inserted on backup job start), it's a kind of "undocumented feature
   use". So I think we are free to change it.

2. There is a hope, that there is no such users: it's a lot more native
   to give a good node-name to backup-top filter if need to operate
   with it somehow, and don't touch format name.

3. Another "incompatible" change in further commit would be moving
   copy-before-write filter from using backing child to file child. And
   this is even more reasonable than renaming: for now all public
   filters are file-child based.

So, it's a risky change, but risk seems small and good interface worth
it.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/{backup-top.h => copy-before-write.h} |  28 +++---
 block/backup.c  |  22 ++---
 block/{backup-top.c => copy-before-write.c} | 100 ++--
 MAINTAINERS |   4 +-
 block/meson.build   |   2 +-
 tests/qemu-iotests/283  |  35 +++
 tests/qemu-iotests/283.out  |   4 +-
 7 files changed, 95 insertions(+), 100 deletions(-)
 rename block/{backup-top.h => copy-before-write.h} (56%)
 rename block/{backup-top.c => copy-before-write.c} (62%)

diff --git a/block/backup-top.h b/block/copy-before-write.h
similarity index 56%
rename from block/backup-top.h
rename to block/copy-before-write.h
index b28b0031c4..5977b7aa31 100644
--- a/block/backup-top.h
+++ b/block/copy-before-write.h
@@ -1,10 +1,10 @@
 /*
- * backup-top filter driver
+ * copy-before-write filter driver
  *
  * The driver performs Copy-Before-Write (CBW) operation: it is injected above
  * some node, and before each write it copies _old_ data to the target node.
  *
- * Copyright (c) 2018-2019 Virtuozzo International GmbH.
+ * Copyright (c) 2018-2021 Virtuozzo International GmbH.
  *
  * Author:
  *  Sementsov-Ogievskiy Vladimir 
@@ -23,20 +23,20 @@
  * along with this program. If not, see .
  */
 
-#ifndef BACKUP_TOP_H
-#define BACKUP_TOP_H
+#ifndef COPY_BEFORE_WRITE_H
+#define COPY_BEFORE_WRITE_H
 
 #include "block/block_int.h"
 #include "block/block-copy.h"
 
-BlockDriverState *bdrv_backup_top_append(BlockDriverState *source,
- BlockDriverState *target,
- const char *filter_node_name,
- uint64_t cluster_size,
- BackupPerf *perf,
- BdrvRequestFlags write_flags,
- BlockCopyState **bcs,
- Error **errp);
-void bdrv_backup_top_drop(BlockDriverState *bs);
+BlockDriverState *bdrv_cbw_append(BlockDriverState *source,
+  BlockDriverState *target,
+  const char *filter_node_name,
+  uint64_t cluster_size,
+  BackupPerf *perf,
+  BdrvRequestFlags write_flags,
+  BlockCopyState **bcs,
+  Error **errp);
+void bdrv_cbw_drop(BlockDriverState *bs);
 
-#endif /* BACKUP_TOP_H */
+#endif /* COPY_BEFORE_WRITE_H */
diff --git a/block/backup.c b/block/backup.c
index bd3614ce70..ac91821b08 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -27,13 +27,13 @@
 #include "qemu/bitmap.h"
 #include "qemu/error-report.h"
 
-#include "block/backup-top.h"
+#include "block/copy-before-write.h"
 
 #define BACKUP_CLUSTER_SIZE_DEFAULT (1 << 16)
 
 typedef struct BackupBlockJob {
 BlockJob common;
-BlockDriverState *backup_top;
+BlockDriverState *cbw;
 BlockDriverState *source_bs;
 BlockDriverState *target_bs;
 
@@ -104,7 +104,7 @@ static void backup_clean(Job *job)
 {
 BackupBlockJob *s = container_of(job, BackupBlockJob, common.job);
 block_job_remove_all_bdrv(&s->common);
-bdrv_backup_top_drop(s->backup_top);
+bdrv_cbw_drop(s->cbw);
 }
 
 void backup_do_checkpoint(BlockJob *job, Error **errp)
@@ -408,7 +408,7 @@ BlockJob *backup_job_create(const char *job_id, 
BlockDriverState *bs,
 BackupBlockJob *job = NULL;
 int64_t cluster_size;
 BdrvRequestFlags wr

[PATCH v2 04/33] block: introduce blk_replace_bs

[Vladimir Sementsov-Ogievskiy]

Add function to change bs inside blk.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
---
 include/sysemu/block-backend.h | 1 +
 block/block-backend.c  | 8 
 2 files changed, 9 insertions(+)

diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 880e903293..aec05ef0a0 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -98,6 +98,7 @@ BlockBackend *blk_by_public(BlockBackendPublic *public);
 BlockDriverState *blk_bs(BlockBackend *blk);
 void blk_remove_bs(BlockBackend *blk);
 int blk_insert_bs(BlockBackend *blk, BlockDriverState *bs, Error **errp);
+int blk_replace_bs(BlockBackend *blk, BlockDriverState *new_bs, Error **errp);
 bool bdrv_has_blk(BlockDriverState *bs);
 bool bdrv_is_root_node(BlockDriverState *bs);
 int blk_set_perm(BlockBackend *blk, uint64_t perm, uint64_t shared_perm,
diff --git a/block/block-backend.c b/block/block-backend.c
index de5496af66..b1abc6f3e6 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -870,6 +870,14 @@ int blk_insert_bs(BlockBackend *blk, BlockDriverState *bs, 
Error **errp)
 return 0;
 }
 
+/*
+ * Change BlockDriverState associated with @blk.
+ */
+int blk_replace_bs(BlockBackend *blk, BlockDriverState *new_bs, Error **errp)
+{
+return bdrv_replace_child_bs(blk->root, new_bs, errp);
+}
+
 /*
  * Sets the permission bitmasks that the user of the BlockBackend needs.
  */
-- 
2.29.2

[PATCH v2 09/33] block-copy: always set BDRV_REQ_SERIALISING flag

[Vladimir Sementsov-Ogievskiy]

It won't hurt in common case, so let's not bother with detecting image
fleecing.

Also, we want to simplify initialization interface of copy-before-write
filter as we are going to make it public.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
---
 block/copy-before-write.h  |  2 +-
 include/block/block-copy.h |  3 +--
 block/backup.c | 20 +---
 block/block-copy.c | 29 ++---
 block/copy-before-write.c  |  4 ++--
 5 files changed, 31 insertions(+), 27 deletions(-)

diff --git a/block/copy-before-write.h b/block/copy-before-write.h
index e284dfb6a7..538aab8bdb 100644
--- a/block/copy-before-write.h
+++ b/block/copy-before-write.h
@@ -33,7 +33,7 @@ BlockDriverState *bdrv_cbw_append(BlockDriverState *source,
   BlockDriverState *target,
   const char *filter_node_name,
   uint64_t cluster_size,
-  BdrvRequestFlags write_flags,
+  bool compress,
   BlockCopyState **bcs,
   Error **errp);
 void bdrv_cbw_drop(BlockDriverState *bs);
diff --git a/include/block/block-copy.h b/include/block/block-copy.h
index 338f2ea7fd..c013a20e1e 100644
--- a/include/block/block-copy.h
+++ b/include/block/block-copy.h
@@ -24,8 +24,7 @@ typedef struct BlockCopyCallState BlockCopyCallState;
 
 BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target,
  int64_t cluster_size, bool use_copy_range,
- BdrvRequestFlags write_flags,
- Error **errp);
+ bool compress, Error **errp);
 
 void block_copy_set_progress_meter(BlockCopyState *s, ProgressMeter *pm);
 
diff --git a/block/backup.c b/block/backup.c
index d41dd30e25..29db3a0ef6 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -407,7 +407,6 @@ BlockJob *backup_job_create(const char *job_id, 
BlockDriverState *bs,
 int64_t len, target_len;
 BackupBlockJob *job = NULL;
 int64_t cluster_size;
-BdrvRequestFlags write_flags;
 BlockDriverState *cbw = NULL;
 BlockCopyState *bcs = NULL;
 
@@ -504,25 +503,8 @@ BlockJob *backup_job_create(const char *job_id, 
BlockDriverState *bs,
 goto error;
 }
 
-/*
- * If source is in backing chain of target assume that target is going to 
be
- * used for "image fleecing", i.e. it should represent a kind of snapshot 
of
- * source at backup-start point in time. And target is going to be read by
- * somebody (for example, used as NBD export) during backup job.
- *
- * In this case, we need to add BDRV_REQ_SERIALISING write flag to avoid
- * intersection of backup writes and third party reads from target,
- * otherwise reading from target we may occasionally read already updated 
by
- * guest data.
- *
- * For more information see commit f8d59dfb40bb and test
- * tests/qemu-iotests/222
- */
-write_flags = (bdrv_chain_contains(target, bs) ? BDRV_REQ_SERIALISING : 0) 
|
-  (compress ? BDRV_REQ_WRITE_COMPRESSED : 0),
-
 cbw = bdrv_cbw_append(bs, target, filter_node_name,
-  cluster_size, write_flags, &bcs, errp);
+  cluster_size, compress, &bcs, errp);
 if (!cbw) {
 goto error;
 }
diff --git a/block/block-copy.c b/block/block-copy.c
index 9b4af00614..daa1a2bf9f 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -245,7 +245,7 @@ static uint32_t block_copy_max_transfer(BdrvChild *source, 
BdrvChild *target)
 
 BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target,
  int64_t cluster_size, bool use_copy_range,
- BdrvRequestFlags write_flags, Error 
**errp)
+ bool compress, Error **errp)
 {
 BlockCopyState *s;
 BdrvDirtyBitmap *copy_bitmap;
@@ -257,6 +257,28 @@ BlockCopyState *block_copy_state_new(BdrvChild *source, 
BdrvChild *target,
 }
 bdrv_disable_dirty_bitmap(copy_bitmap);
 
+/*
+ * Why we always set BDRV_REQ_SERIALISING write flag:
+ *
+ * Assume source is in backing chain of target assume that target is going
+ * to be used for "image fleecing", i.e. it should represent a kind of
+ * snapshot of source at backup-start point in time. And target is going to
+ * be read by somebody (for example, used as NBD export) during backup job.
+ *
+ * In this case, we need to add BDRV_REQ_SERIALISING write flag to avoid
+ * intersection of backup writes and third party reads from target,
+ * otherwise reading from target we may occasionally read already updated 
by
+ * guest data.
+ *
+ * For more information see commit f8d59dfb40bb 

[PATCH v2 05/33] qdev-properties: PropertyInfo: add realized_set_allowed field

[Vladimir Sementsov-Ogievskiy]

Add field, so property can declare support for setting the property
when device is realized. To be used in the following commit.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 include/hw/qdev-properties.h | 1 +
 hw/core/qdev-properties.c| 6 +++---
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index 0ef97d60ce..f7925f67d0 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -32,6 +32,7 @@ struct PropertyInfo {
 const char *name;
 const char *description;
 const QEnumLookup *enum_table;
+bool realized_set_allowed; /* allow setting property on realized device */
 int (*print)(Object *obj, Property *prop, char *dest, size_t len);
 void (*set_default_value)(ObjectProperty *op, const Property *prop);
 ObjectProperty *(*create)(ObjectClass *oc, const char *name,
diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 50f40949f5..c34aac6ebc 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -26,11 +26,11 @@ void qdev_prop_set_after_realize(DeviceState *dev, const 
char *name,
 
 /* returns: true if property is allowed to be set, false otherwise */
 static bool qdev_prop_allow_set(Object *obj, const char *name,
-Error **errp)
+const PropertyInfo *info, Error **errp)
 {
 DeviceState *dev = DEVICE(obj);
 
-if (dev->realized) {
+if (dev->realized && !info->realized_set_allowed) {
 qdev_prop_set_after_realize(dev, name, errp);
 return false;
 }
@@ -79,7 +79,7 @@ static void field_prop_set(Object *obj, Visitor *v, const 
char *name,
 {
 Property *prop = opaque;
 
-if (!qdev_prop_allow_set(obj, name, errp)) {
+if (!qdev_prop_allow_set(obj, name, prop->info, errp)) {
 return;
 }
 
-- 
2.29.2

[PATCH v2 10/33] block/backup: move cluster size calculation to block-copy

[Vladimir Sementsov-Ogievskiy]

The main consumer of cluster-size is block-copy. Let's calculate it
here instead of passing through backup-top.

We are going to publish copy-before-write filter soon, so it will be
created through options. But we don't want for now to make explicit
option for cluster-size, let's continue to calculate it automatically.
So, now is the time to get rid of cluster_size argument for
bdrv_cbw_append().

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
---
 block/copy-before-write.h  |  1 -
 include/block/block-copy.h |  5 +--
 block/backup.c | 62 ++
 block/block-copy.c | 51 ++-
 block/copy-before-write.c  | 10 +++---
 5 files changed, 66 insertions(+), 63 deletions(-)

diff --git a/block/copy-before-write.h b/block/copy-before-write.h
index 538aab8bdb..b386fd8f01 100644
--- a/block/copy-before-write.h
+++ b/block/copy-before-write.h
@@ -32,7 +32,6 @@
 BlockDriverState *bdrv_cbw_append(BlockDriverState *source,
   BlockDriverState *target,
   const char *filter_node_name,
-  uint64_t cluster_size,
   bool compress,
   BlockCopyState **bcs,
   Error **errp);
diff --git a/include/block/block-copy.h b/include/block/block-copy.h
index c013a20e1e..8138686eb4 100644
--- a/include/block/block-copy.h
+++ b/include/block/block-copy.h
@@ -23,8 +23,8 @@ typedef struct BlockCopyState BlockCopyState;
 typedef struct BlockCopyCallState BlockCopyCallState;
 
 BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target,
- int64_t cluster_size, bool use_copy_range,
- bool compress, Error **errp);
+ bool use_copy_range, bool compress,
+ Error **errp);
 
 void block_copy_set_progress_meter(BlockCopyState *s, ProgressMeter *pm);
 
@@ -86,6 +86,7 @@ void block_copy_kick(BlockCopyCallState *call_state);
 void block_copy_call_cancel(BlockCopyCallState *call_state);
 
 BdrvDirtyBitmap *block_copy_dirty_bitmap(BlockCopyState *s);
+int64_t block_copy_cluster_size(BlockCopyState *s);
 void block_copy_set_skip_unallocated(BlockCopyState *s, bool skip);
 
 #endif /* BLOCK_COPY_H */
diff --git a/block/backup.c b/block/backup.c
index 29db3a0ef6..af17149f22 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -29,8 +29,6 @@
 
 #include "block/copy-before-write.h"
 
-#define BACKUP_CLUSTER_SIZE_DEFAULT (1 << 16)
-
 typedef struct BackupBlockJob {
 BlockJob common;
 BlockDriverState *cbw;
@@ -354,43 +352,6 @@ static const BlockJobDriver backup_job_driver = {
 .set_speed = backup_set_speed,
 };
 
-static int64_t backup_calculate_cluster_size(BlockDriverState *target,
- Error **errp)
-{
-int ret;
-BlockDriverInfo bdi;
-bool target_does_cow = bdrv_backing_chain_next(target);
-
-/*
- * If there is no backing file on the target, we cannot rely on COW if our
- * backup cluster size is smaller than the target cluster size. Even for
- * targets with a backing file, try to avoid COW if possible.
- */
-ret = bdrv_get_info(target, &bdi);
-if (ret == -ENOTSUP && !target_does_cow) {
-/* Cluster size is not defined */
-warn_report("The target block device doesn't provide "
-"information about the block size and it doesn't have a "
-"backing file. The default block size of %u bytes is "
-"used. If the actual block size of the target exceeds "
-"this default, the backup may be unusable",
-BACKUP_CLUSTER_SIZE_DEFAULT);
-return BACKUP_CLUSTER_SIZE_DEFAULT;
-} else if (ret < 0 && !target_does_cow) {
-error_setg_errno(errp, -ret,
-"Couldn't determine the cluster size of the target image, "
-"which has no backing file");
-error_append_hint(errp,
-"Aborting, since this may create an unusable destination image\n");
-return ret;
-} else if (ret < 0 && target_does_cow) {
-/* Not fatal; just trudge on ahead. */
-return BACKUP_CLUSTER_SIZE_DEFAULT;
-}
-
-return MAX(BACKUP_CLUSTER_SIZE_DEFAULT, bdi.cluster_size);
-}
-
 BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
   BlockDriverState *target, int64_t speed,
   MirrorSyncMode sync_mode, BdrvDirtyBitmap *sync_bitmap,
@@ -448,11 +409,6 @@ BlockJob *backup_job_create(const char *job_id, 
BlockDriverState *bs,
 return NULL;
 }
 
-cluster_size = backup_calculate_cluster_size(target, errp);
-if (cluster_size < 0) {
-goto error;
-}
-
 if (perf->max_workers < 1) {
 error_setg(errp, "m

[PATCH v2 00/33] block: publish backup-top filter

[Vladimir Sementsov-Ogievskiy]

Hi all!

v2:
01-02: new
03: don't bother with supporting empty child: we should never have such
at this point
05: add comment
06: keep checking conflict with global
add realized_set_allowed to qdev_prop_drive_iothread
07: improve cbw_cbw() name
improve commit message
10: rebased on unchanged backup_calculate_cluster_size(). keep r-b  CHECK ME
12: new
13: drop extra bdrv_unref()
18: add compress local variable
add comment about x-deprecated-compress
19: new, replacement for "[PATCH 17/21] block/block-copy: switch to fully set 
bitmap by default"
22: improve qapi documentation
23-33: test: a lot of refactoring

We have image fleecing scheme to export point-in-time state of active
disk (iotest 222):


  backup(sync=none)
 ┌───┐
 ▼   │
┌┐ ┌┐  backing ┌─┐
│ NBD export │ ─── │ temp qcow2 img │ ───▶ │ active disk │
└┘ └┘  └─┘
 ▲
┌┐   │
│ guest blk  │ ──┘
└┘


Actually, backup job inserts a backup-top filter, so in detail it looks
like:

  backup(sync=none)
 ┌───┐
 ▼   │
┌┐ ┌┐  backing ┌─┐
│ NBD export │ ─── │ temp qcow2 img │ ───▶ │ active disk │
└┘ └┘  └─┘
 ▲   ▲
 │ target│
 │   │
┌┐ ┌┐  backing   │
│ guest blk  │ ──▶ │   backup-top   │ ───┘
└┘ └┘

And job does nothing here. In a new blockdev world user is intended to
operate on node level, and insert/remove filters by hand. Let's get rid
of job in the scheme:

┌┐ ┌┐  backing ┌─┐
│ NBD export │ ─── │ temp qcow2 img │ ───▶ │ active disk │
└┘ └┘  └─┘
 ▲   ▲
 │ target│
 │   │
┌┐ ┌┐  backing   │
│ guest blk  │ ──▶ │   backup-top   │ ───┘
└┘ └┘


The series prepares qom-set to make possible inserting filters above
root node (patches 03-06), rename backup-top to copy-before-write, do
other preparations for publishing the filter, and finally publish it,
add qapi interface and test new fleecing scheme in 222 (first, some
good test refactoring).

Vladimir Sementsov-Ogievskiy (33):
  block: rename bdrv_replace_child to bdrv_replace_child_tran
  block: comment graph-modifying function not updating permissions
  block: introduce bdrv_replace_child_bs()
  block: introduce blk_replace_bs
  qdev-properties: PropertyInfo: add realized_set_allowed field
  qdev: allow setting drive property for realized device
  block: rename backup-top to copy-before-write
  block/backup: drop support for copy_range
  block-copy: always set BDRV_REQ_SERIALISING flag
  block/backup: move cluster size calculation to block-copy
  block/copy-before-write: relax permission requirements when no parents
  block/copy-before-write: drop extra bdrv_unref on failure path
  block/copy-before-write: use file child instead of backing
  block/copy-before-write: bdrv_cbw_append(): replace child at last
  block/copy-before-write: introduce cbw_init()
  block/copy-before-write: cbw_init(): rename variables
  block/copy-before-write: cbw_init(): use file child after attaching
  block/copy-before-write: cbw_init(): use options
  block/copy-before-write: initialize block-copy bitmap
  block/block-copy: make setting progress optional
  block/copy-before-write: make public block driver
  qapi: publish copy-before-write filter
  python/qemu/machine.py: refactor _qemu_args()
  python/qemu/machine: QEMUMachine: improve qmp() method
  iotests.py: VM: add own __enter__ method
  iotests/222: fix pylint and mypy complains
  iotests/222: constantly use single quotes for strings
  iotests: move 222 to tests/image-fleecing
  iotests.py: hmp_qemu_io: support qdev
  iotests/image-fleecing: proper source device
  iotests/image-fleecing: rename tgt_node
  iotests/image-fleecing: prepare for adding new test-case
  iotests/im

[PATCH v2 03/33] block: introduce bdrv_replace_child_bs()

[Vladimir Sementsov-Ogievskiy]

Add function to transactionally replace bs inside BdrvChild.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 include/block/block.h |  2 ++
 block.c   | 31 +++
 2 files changed, 33 insertions(+)

diff --git a/include/block/block.h b/include/block/block.h
index 82185965ff..f9d5fcb108 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -361,6 +361,8 @@ int bdrv_append(BlockDriverState *bs_new, BlockDriverState 
*bs_top,
 Error **errp);
 int bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
   Error **errp);
+int bdrv_replace_child_bs(BdrvChild *child, BlockDriverState *new_bs,
+  Error **errp);
 BlockDriverState *bdrv_insert_node(BlockDriverState *bs, QDict *node_options,
int flags, Error **errp);
 int bdrv_drop_filter(BlockDriverState *bs, Error **errp);
diff --git a/block.c b/block.c
index 8aabfb03ec..39a5d4be90 100644
--- a/block.c
+++ b/block.c
@@ -4969,6 +4969,37 @@ out:
 return ret;
 }
 
+/* Not for empty child */
+int bdrv_replace_child_bs(BdrvChild *child, BlockDriverState *new_bs,
+  Error **errp)
+{
+int ret;
+Transaction *tran = tran_new();
+g_autoptr(GHashTable) found = NULL;
+g_autoptr(GSList) refresh_list = NULL;
+BlockDriverState *old_bs = child->bs;
+
+bdrv_ref(old_bs);
+bdrv_drained_begin(old_bs);
+bdrv_drained_begin(new_bs);
+
+bdrv_replace_child_tran(child, new_bs, tran);
+
+found = g_hash_table_new(NULL, NULL);
+refresh_list = bdrv_topological_dfs(refresh_list, found, old_bs);
+refresh_list = bdrv_topological_dfs(refresh_list, found, new_bs);
+
+ret = bdrv_list_refresh_perms(refresh_list, NULL, tran, errp);
+
+tran_finalize(tran, ret);
+
+bdrv_drained_end(old_bs);
+bdrv_drained_end(new_bs);
+bdrv_unref(old_bs);
+
+return ret;
+}
+
 static void bdrv_delete(BlockDriverState *bs)
 {
 assert(bdrv_op_blocker_is_empty(bs));
-- 
2.29.2

[PATCH v2 01/33] block: rename bdrv_replace_child to bdrv_replace_child_tran

[Vladimir Sementsov-Ogievskiy]

We have bdrv_replace_child() wrapper on bdrv_replace_child_noperm().
But bdrv_replace_child() doesn't update permissions. It's rather
strange, as normally it's expected that foo() should call foo_noperm()
and update permissions.

Let's rename and add comment.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block.c | 14 --
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index 75a82af641..384413c578 100644
--- a/block.c
+++ b/block.c
@@ -2247,12 +2247,14 @@ static TransactionActionDrv bdrv_replace_child_drv = {
 };
 
 /*
- * bdrv_replace_child
+ * bdrv_replace_child_tran
  *
  * Note: real unref of old_bs is done only on commit.
+ *
+ * The function doesn't update permissions, caller is responsible for this.
  */
-static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs,
-   Transaction *tran)
+static void bdrv_replace_child_tran(BdrvChild *child, BlockDriverState *new_bs,
+Transaction *tran)
 {
 BdrvReplaceChildState *s = g_new(BdrvReplaceChildState, 1);
 *s = (BdrvReplaceChildState) {
@@ -4749,7 +4751,7 @@ static void bdrv_remove_filter_or_cow_child_abort(void 
*opaque)
 }
 
 /*
- * We don't have to restore child->bs here to undo bdrv_replace_child()
+ * We don't have to restore child->bs here to undo 
bdrv_replace_child_tran()
  * because that function is transactionable and it registered own 
completion
  * entries in @tran, so .abort() for bdrv_replace_child_safe() will be
  * called automatically.
@@ -4785,7 +4787,7 @@ static void 
bdrv_remove_filter_or_cow_child(BlockDriverState *bs,
 }
 
 if (child->bs) {
-bdrv_replace_child(child, NULL, tran);
+bdrv_replace_child_tran(child, NULL, tran);
 }
 
 s = g_new(BdrvRemoveFilterOrCowChild, 1);
@@ -4825,7 +4827,7 @@ static int bdrv_replace_node_noperm(BlockDriverState 
*from,
c->name, from->node_name);
 return -EPERM;
 }
-bdrv_replace_child(c, to, tran);
+bdrv_replace_child_tran(c, to, tran);
 }
 
 return 0;
-- 
2.29.2

[PATCH v2 02/33] block: comment graph-modifying function not updating permissions

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/block.c b/block.c
index 384413c578..8aabfb03ec 100644
--- a/block.c
+++ b/block.c
@@ -2762,6 +2762,8 @@ static TransactionActionDrv bdrv_attach_child_common_drv 
= {
 
 /*
  * Common part of attaching bdrv child to bs or to blk or to job
+ *
+ * Function doesn't update permissions, caller is responsible for this.
  */
 static int bdrv_attach_child_common(BlockDriverState *child_bs,
 const char *child_name,
@@ -2836,6 +2838,7 @@ static int bdrv_attach_child_common(BlockDriverState 
*child_bs,
 return 0;
 }
 
+/* Function doesn't update permissions, caller is responsible for this. */
 static int bdrv_attach_child_noperm(BlockDriverState *parent_bs,
 BlockDriverState *child_bs,
 const char *child_name,
@@ -3097,6 +3100,8 @@ static BdrvChildRole bdrv_backing_role(BlockDriverState 
*bs)
 /*
  * Sets the bs->backing link of a BDS. A new reference is created; callers
  * which don't need their own reference any more must call bdrv_unref().
+ *
+ * Function doesn't update permissions, caller is responsible for this.
  */
 static int bdrv_set_backing_noperm(BlockDriverState *bs,
BlockDriverState *backing_hd,
@@ -4775,6 +4780,8 @@ static TransactionActionDrv 
bdrv_remove_filter_or_cow_child_drv = {
  * A function to remove backing-chain child of @bs if exists: cow child for
  * format nodes (always .backing) and filter child for filters (may be .file or
  * .backing)
+ *
+ * Function doesn't update permissions, caller is responsible for this.
  */
 static void bdrv_remove_filter_or_cow_child(BlockDriverState *bs,
 Transaction *tran)
-- 
2.29.2

Re: [PULL 03/19] monitor: hmp_qemu_io: acquire aio contex, fix crash

[Max Reitz]

On 20.05.21 15:44, Peter Maydell wrote:

On Fri, 14 May 2021 at 17:45, Max Reitz  wrote:


From: Vladimir Sementsov-Ogievskiy 


Hi; Coverity complains about this code (CID 1453194):


diff --git a/qemu-io.c b/qemu-io.c
index bf902302e9..57f07501df 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -411,6 +411,19 @@ static void prep_fetchline(void *opaque)
  *fetchable= 1;
  }

+static int do_qemuio_command(const char *cmd)
+{
+int ret;
+AioContext *ctx =
+qemuio_blk ? blk_get_aio_context(qemuio_blk) : qemu_get_aio_context();


Here we check whether qemuio_blk is NULL...


+
+aio_context_acquire(ctx);
+ret = qemuio_command(qemuio_blk, cmd);


...but here we pass it to qemuio_command(), which assumes it must
be non-NULL (via calling command() which calls blk_is_available()).

Bug, or false positive ?


It’s a false positive, Vladimir has sent a patch to silence Coverity:

https://lists.nongnu.org/archive/html/qemu-block/2021-05/msg00853.html

Max

Re: [PATCH v2 0/7] block-copy: protect block-copy internal structures

[Vladimir Sementsov-Ogievskiy]

18.05.2021 13:07, Emanuele Giuseppe Esposito wrote:

This serie of patches aims to reduce the usage of the global
AioContexlock in block-copy, by introducing smaller granularity
locks thus on making the block layer thread safe.

This serie depends on Paolo's coroutine_sleep API and my previous
serie that brings thread safety to the smaller API used by block-copy,
like ratelimit, progressmeter abd co-shared-resource.

What's missing for block-copy to be fully thread-safe is fixing
the CoSleep API to allow cross-thread sleep and wakeup.
Paolo is working on it and will post the patches once his new
CoSleep API is accepted.

Patch 1 introduces the .method field instead of .use_copy_range
and .copy_size, so that it can be later used as atomic.
Patch 2-3 provide comments and refactoring in preparation to
the locks added in patch 4 on BlockCopyTask, patch 5-6 on
BlockCopyCallState and 7 BlockCopyState.

Based-on: <20210517100548.28806-1-pbonz...@redhat.com>
Based-on: <20210518094058.25952-1-eespo...@redhat.com>


Hi! I failed to apply this all. Could you please export your branch with your 
patches at some public git repo?


Signed-off-by: Emanuele Giuseppe Esposito 
---
v1 -> v2:
* More field categorized as IN/State/OUT in the various struct, better
   documentation in the structs
* Fix a couple of places where I missed locks [Vladimir, Paolo]


Emanuele Giuseppe Esposito (6):
   block-copy: improve documentation of BlockCopyTask and BlockCopyState
 types and functions
   block-copy: move progress_set_remaining in block_copy_task_end
   block-copy: add a CoMutex to the BlockCopyTask list
   block-copy: add QemuMutex lock for BlockCopyCallState list
   block-copy: atomic .cancelled and .finished fields in
 BlockCopyCallState
   block-copy: protect BlockCopyState .method fields

Paolo Bonzini (1):
   block-copy: streamline choice of copy_range vs. read/write

  block/block-copy.c | 234 +
  1 file changed, 150 insertions(+), 84 deletions(-)




--
Best regards,
Vladimir

Re: [PULL 03/19] monitor: hmp_qemu_io: acquire aio contex, fix crash

[Peter Maydell]

On Fri, 14 May 2021 at 17:45, Max Reitz  wrote:
>
> From: Vladimir Sementsov-Ogievskiy 

Hi; Coverity complains about this code (CID 1453194):

> diff --git a/qemu-io.c b/qemu-io.c
> index bf902302e9..57f07501df 100644
> --- a/qemu-io.c
> +++ b/qemu-io.c
> @@ -411,6 +411,19 @@ static void prep_fetchline(void *opaque)
>  *fetchable= 1;
>  }
>
> +static int do_qemuio_command(const char *cmd)
> +{
> +int ret;
> +AioContext *ctx =
> +qemuio_blk ? blk_get_aio_context(qemuio_blk) : 
> qemu_get_aio_context();

Here we check whether qemuio_blk is NULL...

> +
> +aio_context_acquire(ctx);
> +ret = qemuio_command(qemuio_blk, cmd);

...but here we pass it to qemuio_command(), which assumes it must
be non-NULL (via calling command() which calls blk_is_available()).

Bug, or false positive ?

thanks
-- PMM

[PATCH v4 11/15] qemu-iotests: allow valgrind to read/delete the generated log file

[Emanuele Giuseppe Esposito]

When using -valgrind on the script tests, it generates a log file
in $TEST_DIR that is either read (if valgrind finds problems) or
otherwise deleted. Provide the same exact behavior when using
-valgrind on the python tests.

Signed-off-by: Emanuele Giuseppe Esposito 
---
 tests/qemu-iotests/iotests.py | 16 
 1 file changed, 16 insertions(+)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 5d75094ba6..a06284acad 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -597,6 +597,22 @@ def __init__(self, path_suffix=''):
  sock_dir=sock_dir, qmp_timer=timer)
 self._num_drives = 0
 
+def subprocess_check_valgrind(self, valgrind: List[str]) -> None:
+if not valgrind or not self._popen:
+return
+
+valgrind_filename =  f"{test_dir}/{self._popen.pid}.valgrind"
+
+if self.exitcode() == 99:
+with open(valgrind_filename) as f:
+print(f.read())
+else:
+os.remove(valgrind_filename)
+
+def _post_shutdown(self) -> None:
+super()._post_shutdown()
+self.subprocess_check_valgrind(qemu_valgrind)
+
 def add_object(self, opts):
 self._args.append('-object')
 self._args.append(opts)
-- 
2.30.2

[PATCH v4 14/15] qemu-iotests: add option to show qemu binary logs on stdout

[Emanuele Giuseppe Esposito]

Using the flag -p, allow the qemu binary to print to stdout.

Reviewed-by: Max Reitz 
Signed-off-by: Emanuele Giuseppe Esposito 
---
 tests/qemu-iotests/check  | 4 +++-
 tests/qemu-iotests/iotests.py | 9 +
 tests/qemu-iotests/testenv.py | 9 +++--
 3 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index 2101cedfe3..51b90681ab 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -33,6 +33,8 @@ def make_argparser() -> argparse.ArgumentParser:
help='pretty print output for make check')
 
 p.add_argument('-d', dest='debug', action='store_true', help='debug')
+p.add_argument('-p', dest='print', action='store_true',
+help='redirects qemu\'s stdout and stderr to the test output')
 p.add_argument('-gdb', action='store_true',
help="start gdbserver with $GDB_OPTIONS options \
 ('localhost:12345' if $GDB_OPTIONS is empty)")
@@ -117,7 +119,7 @@ if __name__ == '__main__':
   aiomode=args.aiomode, cachemode=args.cachemode,
   imgopts=args.imgopts, misalign=args.misalign,
   debug=args.debug, valgrind=args.valgrind,
-  gdb=args.gdb)
+  gdb=args.gdb, qprint=args.print)
 
 testfinder = TestFinder(test_dir=env.source_iotests)
 
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 75f1e1711c..53a3916a91 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -80,6 +80,8 @@
 if gdb_qemu_env:
 qemu_gdb = ['gdbserver'] + gdb_qemu_env.strip().split(' ')
 
+qemu_print = os.environ.get('PRINT_QEMU', False)
+
 imgfmt = os.environ.get('IMGFMT', 'raw')
 imgproto = os.environ.get('IMGPROTO', 'file')
 output_dir = os.environ.get('OUTPUT_DIR', '.')
@@ -614,6 +616,13 @@ def _post_shutdown(self) -> None:
 super()._post_shutdown()
 self.subprocess_check_valgrind(qemu_valgrind)
 
+def _pre_launch(self) -> None:
+super()._pre_launch()
+if qemu_print and self._qemu_log_file is not None:
+# set QEMU binary output to stdout
+self._qemu_log_file.close()
+self._qemu_log_file = None
+
 def add_object(self, opts):
 self._args.append('-object')
 self._args.append(opts)
diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
index 319d29cb0c..b79ce22fe9 100644
--- a/tests/qemu-iotests/testenv.py
+++ b/tests/qemu-iotests/testenv.py
@@ -74,7 +74,7 @@ class TestEnv(ContextManager['TestEnv']):
  'AIOMODE', 'CACHEMODE', 'VALGRIND_QEMU',
  'CACHEMODE_IS_DEFAULT', 'IMGFMT_GENERIC', 'IMGOPTSSYNTAX',
  'IMGKEYSECRET', 'QEMU_DEFAULT_MACHINE', 'MALLOC_PERTURB_',
- 'GDB_OPTIONS']
+ 'GDB_OPTIONS', 'PRINT_QEMU']
 
 def get_env(self) -> Dict[str, str]:
 env = {}
@@ -166,7 +166,8 @@ def __init__(self, imgfmt: str, imgproto: str, aiomode: str,
  misalign: bool = False,
  debug: bool = False,
  valgrind: bool = False,
- gdb: bool = False) -> None:
+ gdb: bool = False,
+ qprint: bool = False) -> None:
 self.imgfmt = imgfmt
 self.imgproto = imgproto
 self.aiomode = aiomode
@@ -174,6 +175,9 @@ def __init__(self, imgfmt: str, imgproto: str, aiomode: str,
 self.misalign = misalign
 self.debug = debug
 
+if qprint:
+self.print_qemu = 'y'
+
 if gdb:
 self.gdb_options = os.environ.get('GDB_OPTIONS', DEF_GDB_OPTIONS)
 if not self.gdb_options:
@@ -283,6 +287,7 @@ def print_env(self) -> None:
 SOCKET_SCM_HELPER -- {SOCKET_SCM_HELPER}
 GDB_OPTIONS   -- {GDB_OPTIONS}
 VALGRIND_QEMU -- {VALGRIND_QEMU}
+PRINT_QEMU_OUTPUT -- {PRINT_QEMU}
 """
 
 args = collections.defaultdict(str, self.get_env())
-- 
2.30.2

[PATCH v4 13/15] docs/devel/testing: add -valgrind option to the debug section of QEMU iotests

[Emanuele Giuseppe Esposito]

Reviewed-by: Max Reitz 
Signed-off-by: Emanuele Giuseppe Esposito 
---
 docs/devel/testing.rst | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
index a746cab745..d743e88746 100644
--- a/docs/devel/testing.rst
+++ b/docs/devel/testing.rst
@@ -240,6 +240,13 @@ a failing test:
   If the ``-gdb`` option is not used, ``$GDB_OPTIONS`` is ignored,
   regardless on whether it is set or not.
 
+* ``-valgrind`` attaches a valgrind instance to QEMU. If it detects
+  warnings, it will print and save the log in
+  ``$TEST_DIR/.valgrind``.
+  The final command line will be ``valgrind --log-file=$TEST_DIR/
+  .valgrind --error-exitcode=99 $QEMU ...``
+  Note: if used together with ``-gdb``, this command will be ignored.
+
 * ``-d`` (debug) just increases the logging verbosity, showing
   for example the QMP commands and answers.
 
-- 
2.30.2

[PATCH v4 15/15] docs/devel/testing: add -p option to the debug section of QEMU iotests

[Emanuele Giuseppe Esposito]

Signed-off-by: Emanuele Giuseppe Esposito 
---
 docs/devel/testing.rst | 4 
 1 file changed, 4 insertions(+)

diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
index d743e88746..1192d6489e 100644
--- a/docs/devel/testing.rst
+++ b/docs/devel/testing.rst
@@ -250,6 +250,10 @@ a failing test:
 * ``-d`` (debug) just increases the logging verbosity, showing
   for example the QMP commands and answers.
 
+* ``-p`` (print) redirect QEMU’s stdout and stderr to the test output,
+  instead of saving it into a log file in
+  ``$TEST_DIR/qemu-machine-``.
+
 Test case groups
 
 
-- 
2.30.2

[PATCH v4 09/15] qemu-iotests: extend the check script to support valgrind for python tests

[Emanuele Giuseppe Esposito]

Currently, the check script only parses the option and sets the
VALGRIND_QEMU environmental variable to "y".
Add another local python variable that prepares the command line,
identical to the one provided in the test scripts.

Because the python script does not know in advance the valgring
PID to assign to the log file name, use the "%p" flag in valgrind
log file name that automatically puts the process PID at runtime.

Reviewed-by: Max Reitz 
Signed-off-by: Emanuele Giuseppe Esposito 
---
 tests/qemu-iotests/check  |  7 ---
 tests/qemu-iotests/iotests.py | 11 +++
 tests/qemu-iotests/testenv.py |  1 +
 3 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index b9820fdaaf..2101cedfe3 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -36,6 +36,10 @@ def make_argparser() -> argparse.ArgumentParser:
 p.add_argument('-gdb', action='store_true',
help="start gdbserver with $GDB_OPTIONS options \
 ('localhost:12345' if $GDB_OPTIONS is empty)")
+p.add_argument('-valgrind', action='store_true',
+help='use valgrind, sets VALGRIND_QEMU environment '
+'variable')
+
 p.add_argument('-misalign', action='store_true',
help='misalign memory allocations')
 p.add_argument('--color', choices=['on', 'off', 'auto'],
@@ -86,9 +90,6 @@ def make_argparser() -> argparse.ArgumentParser:
 g_bash.add_argument('-o', dest='imgopts',
 help='options to pass to qemu-img create/convert, '
 'sets IMGOPTS environment variable')
-g_bash.add_argument('-valgrind', action='store_true',
-help='use valgrind, sets VALGRIND_QEMU environment '
-'variable')
 
 g_sel = p.add_argument_group('test selecting options',
  'The following options specify test set '
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index c9628e6828..41462a80fc 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -97,6 +97,17 @@
 sys.stderr.write('Please run this test via the "check" script\n')
 sys.exit(os.EX_USAGE)
 
+qemu_valgrind = []
+if os.environ.get('VALGRIND_QEMU') == "y" and \
+os.environ.get('NO_VALGRIND') != "y":
+valgrind_logfile = "--log-file=" + test_dir.strip()
+# %p allows to put the valgrind process PID, since
+# we don't know it a priori (subprocess.Popen is
+# not yet invoked)
+valgrind_logfile += "/%p.valgrind"
+
+qemu_valgrind = ['valgrind', valgrind_logfile, '--error-exitcode=99']
+
 socket_scm_helper = os.environ.get('SOCKET_SCM_HELPER', 'socket_scm_helper')
 
 luks_default_secret_object = 'secret,id=keysec0,data=' + \
diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
index 49ddd586ef..319d29cb0c 100644
--- a/tests/qemu-iotests/testenv.py
+++ b/tests/qemu-iotests/testenv.py
@@ -282,6 +282,7 @@ def print_env(self) -> None:
 SOCK_DIR  -- {SOCK_DIR}
 SOCKET_SCM_HELPER -- {SOCKET_SCM_HELPER}
 GDB_OPTIONS   -- {GDB_OPTIONS}
+VALGRIND_QEMU -- {VALGRIND_QEMU}
 """
 
 args = collections.defaultdict(str, self.get_env())
-- 
2.30.2

[PATCH v4 05/15] qemu-iotests: delay QMP socket timers

[Emanuele Giuseppe Esposito]

Attaching gdbserver implies that the qmp socket
should wait indefinitely for an answer from QEMU.

Signed-off-by: Emanuele Giuseppe Esposito 
---
 tests/qemu-iotests/iotests.py | 10 ++
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index d667fde6f8..cf1ca60376 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -478,11 +478,13 @@ def __init__(self, seconds, errmsg="Timeout"):
 self.seconds = seconds
 self.errmsg = errmsg
 def __enter__(self):
-signal.signal(signal.SIGALRM, self.timeout)
-signal.setitimer(signal.ITIMER_REAL, self.seconds)
+if not qemu_gdb:
+signal.signal(signal.SIGALRM, self.timeout)
+signal.setitimer(signal.ITIMER_REAL, self.seconds)
 return self
 def __exit__(self, exc_type, value, traceback):
-signal.setitimer(signal.ITIMER_REAL, 0)
+if not qemu_gdb:
+signal.setitimer(signal.ITIMER_REAL, 0)
 return False
 def timeout(self, signum, frame):
 raise Exception(self.errmsg)
@@ -576,7 +578,7 @@ class VM(qtest.QEMUQtestMachine):
 
 def __init__(self, path_suffix=''):
 name = "qemu%s-%d" % (path_suffix, os.getpid())
-timer = 15.0
+timer = 15.0 if not qemu_gdb else None
 super().__init__(qemu_prog, qemu_opts, name=name,
  test_dir=test_dir,
  socket_scm_helper=socket_scm_helper,
-- 
2.30.2

[PATCH v4 07/15] qemu-iotests: add gdbserver option to script tests too

[Emanuele Giuseppe Esposito]

The only limitation here is that running a script with gdbserver
will make the test output mismatch with the expected
results, making the test fail.

Signed-off-by: Emanuele Giuseppe Esposito 
---
 tests/qemu-iotests/common.rc | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index 7f49c9716d..f1d5395ff2 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -166,8 +166,14 @@ _qemu_wrapper()
 if [ -n "${QEMU_NEED_PID}" ]; then
 echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid"
 fi
+
+GDB=""
+if [ ! -z ${GDB_OPTIONS} ]; then
+GDB="gdbserver ${GDB_OPTIONS}"
+fi
+
 VALGRIND_QEMU="${VALGRIND_QEMU_VM}" _qemu_proc_exec 
"${VALGRIND_LOGFILE}" \
-"$QEMU_PROG" $QEMU_OPTIONS "$@"
+$GDB "$QEMU_PROG" $QEMU_OPTIONS "$@"
 )
 RETVAL=$?
 _qemu_proc_valgrind_log "${VALGRIND_LOGFILE}" $RETVAL
-- 
2.30.2

[PATCH v4 10/15] qemu-iotests: extent QMP socket timeout when using valgrind

[Emanuele Giuseppe Esposito]

As with gdbserver, valgrind delays the test execution, so
the default QMP socket timeout timeout too soon.

Signed-off-by: Emanuele Giuseppe Esposito 
---
 tests/qemu-iotests/iotests.py | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 41462a80fc..5d75094ba6 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -489,12 +489,12 @@ def __init__(self, seconds, errmsg="Timeout"):
 self.seconds = seconds
 self.errmsg = errmsg
 def __enter__(self):
-if not qemu_gdb:
+if not (qemu_gdb or qemu_valgrind):
 signal.signal(signal.SIGALRM, self.timeout)
 signal.setitimer(signal.ITIMER_REAL, self.seconds)
 return self
 def __exit__(self, exc_type, value, traceback):
-if not qemu_gdb:
+if not (qemu_gdb or qemu_valgrind):
 signal.setitimer(signal.ITIMER_REAL, 0)
 return False
 def timeout(self, signum, frame):
@@ -589,7 +589,7 @@ class VM(qtest.QEMUQtestMachine):
 
 def __init__(self, path_suffix=''):
 name = "qemu%s-%d" % (path_suffix, os.getpid())
-timer = 15.0 if not qemu_gdb else None
+timer = 15.0 if not (qemu_gdb or qemu_valgrind) else None
 super().__init__(qemu_prog, qemu_opts, wrapper=qemu_gdb,
  name=name,
  test_dir=test_dir,
-- 
2.30.2

[PATCH v4 03/15] docs/devel/testing: add debug section to the QEMU iotests chapter

[Emanuele Giuseppe Esposito]

Introduce the "Debugging a test case" section, in preparation
to the additional flags that will be added in the next patches.

Signed-off-by: Emanuele Giuseppe Esposito 
---
 docs/devel/testing.rst | 8 
 1 file changed, 8 insertions(+)

diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
index 1da4c4e4c4..8144e316a4 100644
--- a/docs/devel/testing.rst
+++ b/docs/devel/testing.rst
@@ -224,6 +224,14 @@ another application on the host may have locked the file, 
possibly leading to a
 test failure.  If using such devices are explicitly desired, consider adding
 ``locking=off`` option to disable image locking.
 
+Debugging a test case
+---
+The following options to the ``check`` script can be useful when debugging
+a failing test:
+
+* ``-d`` (debug) just increases the logging verbosity, showing
+  for example the QMP commands and answers.
+
 Test case groups
 
 
-- 
2.30.2

[PATCH v4 01/15] python: qemu: add timer parameter for qmp.accept socket

[Emanuele Giuseppe Esposito]

Alsp add a new _qmp_timer field to the QEMUMachine class.

Let's change the default socket timeout to None, so that if
a subclass needs to add a timer, it can be done by modifying
this private field.

At the same time, restore the timer to be 15 seconds in iotests.py, to
give an upper bound to qemu-iotests execution.

Signed-off-by: Emanuele Giuseppe Esposito 
---
 python/qemu/machine.py| 7 +--
 python/qemu/qtest.py  | 5 +++--
 tests/qemu-iotests/iotests.py | 3 ++-
 3 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index 6e44bda337..df32de4377 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -89,7 +89,8 @@ def __init__(self,
  socket_scm_helper: Optional[str] = None,
  sock_dir: Optional[str] = None,
  drain_console: bool = False,
- console_log: Optional[str] = None):
+ console_log: Optional[str] = None,
+ qmp_timer: Optional[float] = None):
 '''
 Initialize a QEMUMachine
 
@@ -103,6 +104,7 @@ def __init__(self,
 @param sock_dir: where to create socket (overrides test_dir for sock)
 @param drain_console: (optional) True to drain console socket to buffer
 @param console_log: (optional) path to console log file
+@param qmp_timer: (optional) default QMP socket timeout
 @note: Qemu process is not started until launch() is used.
 '''
 # Direct user configuration
@@ -110,6 +112,7 @@ def __init__(self,
 self._binary = binary
 self._args = list(args)
 self._wrapper = wrapper
+self._qmp_timer = qmp_timer
 
 self._name = name or "qemu-%d" % os.getpid()
 self._test_dir = test_dir
@@ -323,7 +326,7 @@ def _pre_launch(self) -> None:
 
 def _post_launch(self) -> None:
 if self._qmp_connection:
-self._qmp.accept()
+self._qmp.accept(self._qmp_timer)
 
 def _post_shutdown(self) -> None:
 """
diff --git a/python/qemu/qtest.py b/python/qemu/qtest.py
index 39a0cf62fe..afea210d9d 100644
--- a/python/qemu/qtest.py
+++ b/python/qemu/qtest.py
@@ -114,14 +114,15 @@ def __init__(self,
  name: Optional[str] = None,
  test_dir: str = "/var/tmp",
  socket_scm_helper: Optional[str] = None,
- sock_dir: Optional[str] = None):
+ sock_dir: Optional[str] = None,
+ qmp_timer: Optional[float] = None):
 if name is None:
 name = "qemu-%d" % os.getpid()
 if sock_dir is None:
 sock_dir = test_dir
 super().__init__(binary, args, name=name, test_dir=test_dir,
  socket_scm_helper=socket_scm_helper,
- sock_dir=sock_dir)
+ sock_dir=sock_dir, qmp_timer=qmp_timer)
 self._qtest: Optional[QEMUQtestProtocol] = None
 self._qtest_path = os.path.join(sock_dir, name + "-qtest.sock")
 
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index ec3c69daf1..5d78de0f0b 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -571,10 +571,11 @@ class VM(qtest.QEMUQtestMachine):
 
 def __init__(self, path_suffix=''):
 name = "qemu%s-%d" % (path_suffix, os.getpid())
+timer = 15.0
 super().__init__(qemu_prog, qemu_opts, name=name,
  test_dir=test_dir,
  socket_scm_helper=socket_scm_helper,
- sock_dir=sock_dir)
+ sock_dir=sock_dir, qmp_timer=timer)
 self._num_drives = 0
 
 def add_object(self, opts):
-- 
2.30.2

[PATCH v4 12/15] qemu-iotests: insert valgrind command line as wrapper for qemu binary

[Emanuele Giuseppe Esposito]

The priority will be given to gdb command line, meaning if the -gdb
parameter and -valgrind are given, gdb will be wrapped around
the qemu binary.

Signed-off-by: Emanuele Giuseppe Esposito 
---
 tests/qemu-iotests/iotests.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index a06284acad..75f1e1711c 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -590,7 +590,8 @@ class VM(qtest.QEMUQtestMachine):
 def __init__(self, path_suffix=''):
 name = "qemu%s-%d" % (path_suffix, os.getpid())
 timer = 15.0 if not (qemu_gdb or qemu_valgrind) else None
-super().__init__(qemu_prog, qemu_opts, wrapper=qemu_gdb,
+wrapper = qemu_gdb if qemu_gdb else qemu_valgrind
+super().__init__(qemu_prog, qemu_opts, wrapper=wrapper,
  name=name,
  test_dir=test_dir,
  socket_scm_helper=socket_scm_helper,
-- 
2.30.2

[PATCH v4 08/15] docs/devel/testing: add -gdb option to the debugging section of QEMU iotests

[Emanuele Giuseppe Esposito]

Signed-off-by: Emanuele Giuseppe Esposito 
---
 docs/devel/testing.rst | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
index 8144e316a4..a746cab745 100644
--- a/docs/devel/testing.rst
+++ b/docs/devel/testing.rst
@@ -229,6 +229,17 @@ Debugging a test case
 The following options to the ``check`` script can be useful when debugging
 a failing test:
 
+* ``-gdb`` wraps every QEMU invocation in a ``gdbserver``, which waits for a
+  connection from a gdb client.  The options given to ``gdbserver`` (e.g. the
+  address on which to listen for connections) are taken from the 
``$GDB_OPTIONS``
+  environment variable.  By default (if ``$GDB_OPTIONS`` is empty), it listens 
on
+  ``localhost:12345``.
+  It is possible to connect to it for example with
+  ``gdb -iex "target remote $addr"``, where ``$addr`` is the address
+  ``gdbserver`` listens on.
+  If the ``-gdb`` option is not used, ``$GDB_OPTIONS`` is ignored,
+  regardless on whether it is set or not.
+
 * ``-d`` (debug) just increases the logging verbosity, showing
   for example the QMP commands and answers.
 
-- 
2.30.2

[PATCH v4 06/15] qemu_iotests: insert gdbserver command line as wrapper for qemu binary

[Emanuele Giuseppe Esposito]

Signed-off-by: Emanuele Giuseppe Esposito 
---
 tests/qemu-iotests/iotests.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index cf1ca60376..c9628e6828 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -579,7 +579,8 @@ class VM(qtest.QEMUQtestMachine):
 def __init__(self, path_suffix=''):
 name = "qemu%s-%d" % (path_suffix, os.getpid())
 timer = 15.0 if not qemu_gdb else None
-super().__init__(qemu_prog, qemu_opts, name=name,
+super().__init__(qemu_prog, qemu_opts, wrapper=qemu_gdb,
+ name=name,
  test_dir=test_dir,
  socket_scm_helper=socket_scm_helper,
  sock_dir=sock_dir, qmp_timer=timer)
-- 
2.30.2

[PATCH v4 04/15] qemu-iotests: add option to attach gdbserver

[Emanuele Giuseppe Esposito]

Define -gdb flag and GDB_OPTIONS environment variable
to python tests to attach a gdbserver to each qemu instance.
This patch only adds and parses this flag, it does not yet add
the implementation for it.

if -gdb is not provided but $GDB_OPTIONS is set, ignore the
environment variable.

Signed-off-by: Emanuele Giuseppe Esposito 
---
 tests/qemu-iotests/check  |  6 +-
 tests/qemu-iotests/iotests.py |  5 +
 tests/qemu-iotests/testenv.py | 19 ---
 3 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index d1c87ceaf1..b9820fdaaf 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -33,6 +33,9 @@ def make_argparser() -> argparse.ArgumentParser:
help='pretty print output for make check')
 
 p.add_argument('-d', dest='debug', action='store_true', help='debug')
+p.add_argument('-gdb', action='store_true',
+   help="start gdbserver with $GDB_OPTIONS options \
+('localhost:12345' if $GDB_OPTIONS is empty)")
 p.add_argument('-misalign', action='store_true',
help='misalign memory allocations')
 p.add_argument('--color', choices=['on', 'off', 'auto'],
@@ -112,7 +115,8 @@ if __name__ == '__main__':
 env = TestEnv(imgfmt=args.imgfmt, imgproto=args.imgproto,
   aiomode=args.aiomode, cachemode=args.cachemode,
   imgopts=args.imgopts, misalign=args.misalign,
-  debug=args.debug, valgrind=args.valgrind)
+  debug=args.debug, valgrind=args.valgrind,
+  gdb=args.gdb)
 
 testfinder = TestFinder(test_dir=env.source_iotests)
 
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 5d78de0f0b..d667fde6f8 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -75,6 +75,11 @@
 qemu_prog = os.environ.get('QEMU_PROG', 'qemu')
 qemu_opts = os.environ.get('QEMU_OPTIONS', '').strip().split(' ')
 
+gdb_qemu_env = os.environ.get('GDB_OPTIONS')
+qemu_gdb = []
+if gdb_qemu_env:
+qemu_gdb = ['gdbserver'] + gdb_qemu_env.strip().split(' ')
+
 imgfmt = os.environ.get('IMGFMT', 'raw')
 imgproto = os.environ.get('IMGPROTO', 'file')
 output_dir = os.environ.get('OUTPUT_DIR', '.')
diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
index 6d27712617..49ddd586ef 100644
--- a/tests/qemu-iotests/testenv.py
+++ b/tests/qemu-iotests/testenv.py
@@ -27,6 +27,7 @@
 import glob
 from typing import Dict, Any, Optional, ContextManager
 
+DEF_GDB_OPTIONS = 'localhost:12345'
 
 def isxfile(path: str) -> bool:
 return os.path.isfile(path) and os.access(path, os.X_OK)
@@ -72,7 +73,8 @@ class TestEnv(ContextManager['TestEnv']):
  'QEMU_NBD_OPTIONS', 'IMGOPTS', 'IMGFMT', 'IMGPROTO',
  'AIOMODE', 'CACHEMODE', 'VALGRIND_QEMU',
  'CACHEMODE_IS_DEFAULT', 'IMGFMT_GENERIC', 'IMGOPTSSYNTAX',
- 'IMGKEYSECRET', 'QEMU_DEFAULT_MACHINE', 'MALLOC_PERTURB_']
+ 'IMGKEYSECRET', 'QEMU_DEFAULT_MACHINE', 'MALLOC_PERTURB_',
+ 'GDB_OPTIONS']
 
 def get_env(self) -> Dict[str, str]:
 env = {}
@@ -163,7 +165,8 @@ def __init__(self, imgfmt: str, imgproto: str, aiomode: str,
  imgopts: Optional[str] = None,
  misalign: bool = False,
  debug: bool = False,
- valgrind: bool = False) -> None:
+ valgrind: bool = False,
+ gdb: bool = False) -> None:
 self.imgfmt = imgfmt
 self.imgproto = imgproto
 self.aiomode = aiomode
@@ -171,6 +174,14 @@ def __init__(self, imgfmt: str, imgproto: str, aiomode: 
str,
 self.misalign = misalign
 self.debug = debug
 
+if gdb:
+self.gdb_options = os.environ.get('GDB_OPTIONS', DEF_GDB_OPTIONS)
+if not self.gdb_options:
+# cover the case 'export GDB_OPTIONS='
+self.gdb_options = DEF_GDB_OPTIONS
+elif 'GDB_OPTIONS' in os.environ:
+del os.environ['GDB_OPTIONS']
+
 if valgrind:
 self.valgrind_qemu = 'y'
 
@@ -269,7 +280,9 @@ def print_env(self) -> None:
 PLATFORM  -- {platform}
 TEST_DIR  -- {TEST_DIR}
 SOCK_DIR  -- {SOCK_DIR}
-SOCKET_SCM_HELPER -- {SOCKET_SCM_HELPER}"""
+SOCKET_SCM_HELPER -- {SOCKET_SCM_HELPER}
+GDB_OPTIONS   -- {GDB_OPTIONS}
+"""
 
 args = collections.defaultdict(str, self.get_env())
 
-- 
2.30.2

[PATCH v4 02/15] python: qemu: pass the wrapper field from QEMUQtestmachine to QEMUMachine

[Emanuele Giuseppe Esposito]

Reviewed-by: John Snow 
Reviewed-by: Max Reitz 
Signed-off-by: Emanuele Giuseppe Esposito 
---
 python/qemu/qtest.py | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/python/qemu/qtest.py b/python/qemu/qtest.py
index afea210d9d..e6a8fb5984 100644
--- a/python/qemu/qtest.py
+++ b/python/qemu/qtest.py
@@ -111,6 +111,7 @@ class QEMUQtestMachine(QEMUMachine):
 def __init__(self,
  binary: str,
  args: Sequence[str] = (),
+ wrapper: Sequence[str] = (),
  name: Optional[str] = None,
  test_dir: str = "/var/tmp",
  socket_scm_helper: Optional[str] = None,
@@ -120,7 +121,8 @@ def __init__(self,
 name = "qemu-%d" % os.getpid()
 if sock_dir is None:
 sock_dir = test_dir
-super().__init__(binary, args, name=name, test_dir=test_dir,
+super().__init__(binary, args, wrapper=wrapper, name=name,
+ test_dir=test_dir,
  socket_scm_helper=socket_scm_helper,
  sock_dir=sock_dir, qmp_timer=qmp_timer)
 self._qtest: Optional[QEMUQtestProtocol] = None
-- 
2.30.2

[PATCH v4 00/15] qemu_iotests: improve debugging options

[Emanuele Giuseppe Esposito]

This series adds the option to attach gdbserver and valgrind
to the QEMU binary running in qemu_iotests.
It also allows to redirect QEMU binaries output of the python tests
to the stdout, instead of a log file.

Patches 1-6 introduce the -gdb option to both python and bash tests, 
7-10 extend the already existing -valgrind flag to work also on 
python tests, and patch 11 introduces -p to enable logging to stdout.

In particular, patches 1,5,10 focus on extending the QMP socket timers
when using gdb/valgrind, otherwise the python tests will fail due to
delays in the QMP responses.

This series is tested on the previous serie
"qemu-iotests: quality of life improvements"
but independent from it, so it can be applied separately.

Signed-off-by: Emanuele Giuseppe Esposito 
---
v4:
* Rename environment variable from GDB_QEMU to GDB_OPTIONS
* This time test 297 (pylint) passes [Max]
* Refactor the qmp_timer field in machine.py, and add a new 
  parameter in machine.py and subclasses constructor [John]
* Add additional check in patch 4 to cover the case where
  GDB_OPTIONS is empty

Emanuele Giuseppe Esposito (15):
  python: qemu: add timer parameter for qmp.accept socket
  python: qemu: pass the wrapper field from QEMUQtestmachine to
QEMUMachine
  docs/devel/testing: add debug section to the QEMU iotests chapter
  qemu-iotests: add option to attach gdbserver
  qemu-iotests: delay QMP socket timers
  qemu_iotests: insert gdbserver command line as wrapper for qemu binary
  qemu-iotests: add gdbserver option to script tests too
  docs/devel/testing: add -gdb option to the debugging section of QEMU
iotests
  qemu-iotests: extend the check script to support valgrind for python
tests
  qemu-iotests: extent QMP socket timeout when using valgrind
  qemu-iotests: allow valgrind to read/delete the generated log file
  qemu-iotests: insert valgrind command line as wrapper for qemu binary
  docs/devel/testing: add -valgrind option to the debug section of QEMU
iotests
  qemu-iotests: add option to show qemu binary logs on stdout
  docs/devel/testing: add -p option to the debug section of QEMU iotests

 docs/devel/testing.rst| 30 +++
 python/qemu/machine.py|  7 +++--
 python/qemu/qtest.py  |  9 --
 tests/qemu-iotests/check  | 15 +++---
 tests/qemu-iotests/common.rc  |  8 -
 tests/qemu-iotests/iotests.py | 56 +++
 tests/qemu-iotests/testenv.py | 25 ++--
 7 files changed, 132 insertions(+), 18 deletions(-)

-- 
2.30.2