[RFC 00/15] block job API

[RFC 00/15] block job API

[Vladimir Sementsov-Ogievskiy]

Hi all!

The series aims to reach feature parity between job-* and block-job-*
APIs and finally deprecate block-job-* APIs.

01: new type-based unions for QAPI

02-07: rework block-job-change and add similar job-change command

08: support set-speed in new API (as an option to job-change)

09: support soft-cancelling ready mirror job in new API (as an
    option to job-complete)

10-14: prepare query-jobs to substitute query-block-jobs

15: finally, deprecate old APIs

RFC: the series are untested. I plan to move tests to using new APIs
instead of deprecated ones in v2.

Vladimir Sementsov-Ogievskiy (15):
  scripts/qapi: support type-based unions
  qapi: rename BlockJobChangeOptions to JobChangeOptions
  blockjob: block_job_change_locked(): check job type
  qapi: block-job-change: make copy-mode parameter optional
  qapi: JobChangeOptions: make type member optional and deprecated
  blockjob: move change action implementation to job from block-job
  qapi: add job-change
  qapi: job-change: support speed parameter
  qapi: job-complete: introduce no-block-replace option for mirror
  qapi: query-jobs: add information specific for job type
  job-qmp: job_query_single_locked: add assertion on job ret
  qapi: rename BlockDeviceIoStatus to IoStatus
  qapi: move IoStatus to common.json
  qapi: query-job: add block-job specific information
  qapi/block-core: derpecate block-job- APIs

 block/backup.c                              |  14 ++
 block/block-backend.c                       |  14 +-
 block/commit.c                              |  16 ++-
 block/mirror.c                              |  98 +++++++++-----
 block/monitor/block-hmp-cmds.c              |   4 +-
 block/stream.c                              |  14 ++
 blockdev.c                                  |   4 +-
 blockjob.c                                  |  39 +++---
 docs/about/deprecated.rst                   |  75 +++++++++++
 include/block/blockjob.h                    |  26 ++--
 include/block/blockjob_int.h                |   7 -
 include/qemu/job.h                          |  25 +++-
 include/sysemu/block-backend-global-state.h |   2 +-
 job-qmp.c                                   |  55 +++++++-
 job.c                                       |  35 ++++-
 qapi/block-core.json                        | 136 +++++++++++++++-----
 qapi/common.json                            |  16 +++
 qapi/job.json                               |  69 +++++++++-
 scripts/qapi/introspect.py                  |   5 +-
 scripts/qapi/schema.py                      |  50 ++++---
 scripts/qapi/types.py                       |   3 +-
 scripts/qapi/visit.py                       |  43 ++++++-
 stubs/meson.build                           |   1 +
 stubs/qapi-jobchangeoptions-mapper.c        |  13 ++
 tests/unit/test-bdrv-drain.c                |   2 +-
 tests/unit/test-block-iothread.c            |   2 +-
 tests/unit/test-blockjob.c                  |   2 +-
 27 files changed, 616 insertions(+), 154 deletions(-)
 create mode 100644 stubs/qapi-jobchangeoptions-mapper.c

-- 
2.34.1

[RFC 01/15] scripts/qapi: support type-based unions

[Vladimir Sementsov-Ogievskiy]

Look at block-job-change command: we have to specify both 'id' to chose
the job to operate on and 'type' for QAPI union be parsed. But for user
this looks redundant: when we specify 'id', QEMU should be able to get
corresponding job's type.

This commit brings such a possibility: just specify some Enum type as
'discriminator', and define a function somewhere with prototype

bool YourUnionType_mapper(YourUnionType *, EnumType *out, Error **errp)

Further commits will use this functionality to upgrade block-job-change
interface and introduce some new interfaces.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 scripts/qapi/introspect.py |  5 +++-
 scripts/qapi/schema.py     | 50 +++++++++++++++++++++++---------------
 scripts/qapi/types.py      |  3 ++-
 scripts/qapi/visit.py      | 43 ++++++++++++++++++++++++++------
 4 files changed, 73 insertions(+), 28 deletions(-)

diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 67c7d89aae..04d8d424f5 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -336,7 +336,10 @@ def visit_object_type_flat(self, name: str, info: Optional[QAPISourceInfo],
             'members': [self._gen_object_member(m) for m in members]
         }
         if variants:
-            obj['tag'] = variants.tag_member.name
+            if variants.tag_member:
+                obj['tag'] = variants.tag_member.name
+            else:
+                obj['discriminator-type'] = variants.tag_type.name
             obj['variants'] = [self._gen_variant(v) for v in variants.variants]
         self._gen_tree(name, 'object', obj, ifcond, features)
 
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 8ba5665bc6..0efe8d815c 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -585,16 +585,16 @@ def visit(self, visitor):
 
 
 class QAPISchemaVariants:
-    def __init__(self, tag_name, info, tag_member, variants):
+    def __init__(self, discriminator, info, tag_member, variants):
         # Unions pass tag_name but not tag_member.
         # Alternates pass tag_member but not tag_name.
         # After check(), tag_member is always set.
-        assert bool(tag_member) != bool(tag_name)
-        assert (isinstance(tag_name, str) or
+        assert bool(tag_member) != bool(discriminator)
+        assert (isinstance(discriminator, str) or
                 isinstance(tag_member, QAPISchemaObjectTypeMember))
         for v in variants:
             assert isinstance(v, QAPISchemaVariant)
-        self._tag_name = tag_name
+        self.discriminator = discriminator
         self.info = info
         self.tag_member = tag_member
         self.variants = variants
@@ -604,16 +604,24 @@ def set_defined_in(self, name):
             v.set_defined_in(name)
 
     def check(self, schema, seen):
-        if self._tag_name:      # union
-            self.tag_member = seen.get(c_name(self._tag_name))
+        self.tag_type = None
+
+        if self.discriminator:      # assume union with type discriminator
+            self.tag_type = schema.lookup_type(self.discriminator)
+
+        # union with discriminator field
+        if self.discriminator and not self.tag_type:
+            tag_name = self.discriminator
+            self.tag_member = seen.get(c_name(tag_name))
+            self.tag_type = self.tag_member.type
             base = "'base'"
             # Pointing to the base type when not implicit would be
             # nice, but we don't know it here
-            if not self.tag_member or self._tag_name != self.tag_member.name:
+            if not self.tag_member or tag_name != self.tag_member.name:
                 raise QAPISemError(
                     self.info,
                     "discriminator '%s' is not a member of %s"
-                    % (self._tag_name, base))
+                    % (tag_name, base))
             # Here we do:
             base_type = schema.lookup_type(self.tag_member.defined_in)
             assert base_type
@@ -623,29 +631,33 @@ def check(self, schema, seen):
                 raise QAPISemError(
                     self.info,
                     "discriminator member '%s' of %s must be of enum type"
-                    % (self._tag_name, base))
+                    % (tag_name, base))
             if self.tag_member.optional:
                 raise QAPISemError(
                     self.info,
                     "discriminator member '%s' of %s must not be optional"
-                    % (self._tag_name, base))
+                    % (tag_name, base))
             if self.tag_member.ifcond.is_present():
                 raise QAPISemError(
                     self.info,
                     "discriminator member '%s' of %s must not be conditional"
-                    % (self._tag_name, base))
-        else:                   # alternate
+                    % (tag_name, base))
+        elif not self.tag_type:  # alternate
             assert isinstance(self.tag_member.type, QAPISchemaEnumType)
             assert not self.tag_member.optional
             assert not self.tag_member.ifcond.is_present()
-        if self._tag_name:      # union
+
+        if self.tag_type:      # union
             # branches that are not explicitly covered get an empty type
             cases = {v.name for v in self.variants}
-            for m in self.tag_member.type.members:
+            for m in self.tag_type.members:
                 if m.name not in cases:
                     v = QAPISchemaVariant(m.name, self.info,
                                           'q_empty', m.ifcond)
-                    v.set_defined_in(self.tag_member.defined_in)
+                    if self.tag_member:
+                        v.set_defined_in(self.tag_member.defined_in)
+                    else:
+                        v.set_defined_in(self.tag_type.name)
                     self.variants.append(v)
         if not self.variants:
             raise QAPISemError(self.info, "union has no branches")
@@ -654,11 +666,11 @@ def check(self, schema, seen):
             # Union names must match enum values; alternate names are
             # checked separately. Use 'seen' to tell the two apart.
             if seen:
-                if v.name not in self.tag_member.type.member_names():
+                if v.name not in self.tag_type.member_names():
                     raise QAPISemError(
                         self.info,
                         "branch '%s' is not a value of %s"
-                        % (v.name, self.tag_member.type.describe()))
+                        % (v.name, self.tag_type.describe()))
                 if not isinstance(v.type, QAPISchemaObjectType):
                     raise QAPISemError(
                         self.info,
@@ -1116,7 +1128,7 @@ def _make_variant(self, case, typ, ifcond, info):
     def _def_union_type(self, expr: QAPIExpression):
         name = expr['union']
         base = expr['base']
-        tag_name = expr['discriminator']
+        discriminator = expr['discriminator']
         data = expr['data']
         assert isinstance(data, dict)
         info = expr.info
@@ -1136,7 +1148,7 @@ def _def_union_type(self, expr: QAPIExpression):
             QAPISchemaObjectType(name, info, expr.doc, ifcond, features,
                                  base, members,
                                  QAPISchemaVariants(
-                                     tag_name, info, None, variants)))
+                                     discriminator, info, None, variants)))
 
     def _def_alternate_type(self, expr: QAPIExpression):
         name = expr['alternate']
diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
index c39d054d2c..13021e3f82 100644
--- a/scripts/qapi/types.py
+++ b/scripts/qapi/types.py
@@ -230,7 +230,8 @@ def gen_variants(variants: QAPISchemaVariants) -> str:
     ret = mcgen('''
     union { /* union tag is @%(c_name)s */
 ''',
-                c_name=c_name(variants.tag_member.name))
+                c_name=c_name(variants.tag_member.name if variants.tag_member
+                              else variants.tag_type.name))
 
     for var in variants.variants:
         if var.type.name == 'q_empty':
diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
index c56ea4d724..fbe7ae071d 100644
--- a/scripts/qapi/visit.py
+++ b/scripts/qapi/visit.py
@@ -60,6 +60,13 @@ def gen_visit_members_decl(name: str) -> str:
                  c_name=c_name(name))
 
 
+def gen_visit_members_tag_mapper_decl(name: str, tag_type: str) -> str:
+    return mcgen('''
+
+bool %(c_name)s_mapper(%(c_name)s *obj, %(tag_type)s *tag, Error **errp);
+                 ''', c_name=c_name(name), tag_type=c_name(tag_type))
+
+
 def gen_visit_object_members(name: str,
                              base: Optional[QAPISchemaObjectType],
                              members: List[QAPISchemaObjectTypeMember],
@@ -83,6 +90,12 @@ def gen_visit_object_members(name: str,
             ret += memb.ifcond.gen_endif()
     ret += sep
 
+    if variants:
+        ret += mcgen('''
+    %(c_name)s tag;
+''',
+                     c_name=c_name(variants.tag_type.name))
+
     if base:
         ret += mcgen('''
     if (!visit_type_%(c_type)s_members(v, (%(c_type)s *)obj, errp)) {
@@ -132,17 +145,29 @@ def gen_visit_object_members(name: str,
         ret += memb.ifcond.gen_endif()
 
     if variants:
-        tag_member = variants.tag_member
-        assert isinstance(tag_member.type, QAPISchemaEnumType)
+        tag_type = variants.tag_type
+        assert isinstance(tag_type, QAPISchemaEnumType)
 
-        ret += mcgen('''
-    switch (obj->%(c_name)s) {
+        if variants.tag_member:
+            ret += mcgen('''
+    tag = obj->%(c_name)s;
 ''',
-                     c_name=c_name(tag_member.name))
+                         c_name=c_name(variants.tag_member.name))
+        else:
+            ret += mcgen('''
+    if (!%(c_name)s_mapper(obj, &tag, errp)) {
+        return false;
+    }
+''',
+                         c_name=c_name(name))
+
+        ret += mcgen('''
+    switch (tag) {
+''')
 
         for var in variants.variants:
-            case_str = c_enum_const(tag_member.type.name, var.name,
-                                    tag_member.type.prefix)
+            case_str = c_enum_const(tag_type.name, var.name,
+                                    tag_type.prefix)
             ret += var.ifcond.gen_if()
             if var.type.name == 'q_empty':
                 # valid variant and nothing to do
@@ -400,6 +425,10 @@ def visit_object_type(self,
             return
         with ifcontext(ifcond, self._genh, self._genc):
             self._genh.add(gen_visit_members_decl(name))
+            if variants and variants.tag_type:
+                self._genh.add(
+                    gen_visit_members_tag_mapper_decl(name,
+                                                      variants.tag_type.name))
             self._genc.add(gen_visit_object_members(name, base,
                                                     members, variants))
             # TODO Worth changing the visitor signature, so we could
-- 
2.34.1

[RFC 02/15] qapi: rename BlockJobChangeOptions to JobChangeOptions

[Vladimir Sementsov-Ogievskiy]

We are going to move change action from block-job to job
implementation, and then move to job-* extenral APIs, deprecating
block-job-* APIs. This commit simplifies further transition.

The commit is made by command

    git grep -l BlockJobChangeOptions | \
        xargs sed -i 's/BlockJobChangeOptions/JobChangeOptions/g'

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 block/mirror.c               |  4 ++--
 blockdev.c                   |  2 +-
 blockjob.c                   |  2 +-
 include/block/blockjob.h     |  2 +-
 include/block/blockjob_int.h |  2 +-
 qapi/block-core.json         | 12 ++++++------
 6 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index 5145eb53e1..a177502e4f 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1251,11 +1251,11 @@ static bool commit_active_cancel(Job *job, bool force)
     return force || !job_is_ready(job);
 }
 
-static void mirror_change(BlockJob *job, BlockJobChangeOptions *opts,
+static void mirror_change(BlockJob *job, JobChangeOptions *opts,
                           Error **errp)
 {
     MirrorBlockJob *s = container_of(job, MirrorBlockJob, common);
-    BlockJobChangeOptionsMirror *change_opts = &opts->u.mirror;
+    JobChangeOptionsMirror *change_opts = &opts->u.mirror;
     MirrorCopyMode current;
 
     /*
diff --git a/blockdev.c b/blockdev.c
index d8fb3399f5..7881f6e5a6 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3245,7 +3245,7 @@ void qmp_block_job_dismiss(const char *id, Error **errp)
     job_dismiss_locked(&job, errp);
 }
 
-void qmp_block_job_change(BlockJobChangeOptions *opts, Error **errp)
+void qmp_block_job_change(JobChangeOptions *opts, Error **errp)
 {
     BlockJob *job;
 
diff --git a/blockjob.c b/blockjob.c
index d5f29e14af..8cfbb15543 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -312,7 +312,7 @@ static bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)
     return block_job_set_speed_locked(job, speed, errp);
 }
 
-void block_job_change_locked(BlockJob *job, BlockJobChangeOptions *opts,
+void block_job_change_locked(BlockJob *job, JobChangeOptions *opts,
                              Error **errp)
 {
     const BlockJobDriver *drv = block_job_driver(job);
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 7061ab7201..5dd1b08909 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -181,7 +181,7 @@ bool block_job_set_speed_locked(BlockJob *job, int64_t speed, Error **errp);
  *
  * Change the job according to opts.
  */
-void block_job_change_locked(BlockJob *job, BlockJobChangeOptions *opts,
+void block_job_change_locked(BlockJob *job, JobChangeOptions *opts,
                              Error **errp);
 
 /**
diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
index 4c3d2e25a2..d9c3b911d0 100644
--- a/include/block/blockjob_int.h
+++ b/include/block/blockjob_int.h
@@ -73,7 +73,7 @@ struct BlockJobDriver {
      *
      * Note that this can already be called before the job coroutine is running.
      */
-    void (*change)(BlockJob *job, BlockJobChangeOptions *opts, Error **errp);
+    void (*change)(BlockJob *job, JobChangeOptions *opts, Error **errp);
 
     /*
      * Query information specific to this kind of block job.
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 1874f880a8..67dd0ef038 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3063,18 +3063,18 @@
   'allow-preconfig': true }
 
 ##
-# @BlockJobChangeOptionsMirror:
+# @JobChangeOptionsMirror:
 #
 # @copy-mode: Switch to this copy mode.  Currently, only the switch
 #     from 'background' to 'write-blocking' is implemented.
 #
 # Since: 8.2
 ##
-{ 'struct': 'BlockJobChangeOptionsMirror',
+{ 'struct': 'JobChangeOptionsMirror',
   'data': { 'copy-mode' : 'MirrorCopyMode' } }
 
 ##
-# @BlockJobChangeOptions:
+# @JobChangeOptions:
 #
 # Block job options that can be changed after job creation.
 #
@@ -3084,10 +3084,10 @@
 #
 # Since: 8.2
 ##
-{ 'union': 'BlockJobChangeOptions',
+{ 'union': 'JobChangeOptions',
   'base': { 'id': 'str', 'type': 'JobType' },
   'discriminator': 'type',
-  'data': { 'mirror': 'BlockJobChangeOptionsMirror' } }
+  'data': { 'mirror': 'JobChangeOptionsMirror' } }
 
 ##
 # @block-job-change:
@@ -3097,7 +3097,7 @@
 # Since: 8.2
 ##
 { 'command': 'block-job-change',
-  'data': 'BlockJobChangeOptions', 'boxed': true }
+  'data': 'JobChangeOptions', 'boxed': true }
 
 ##
 # @BlockdevDiscardOptions:
-- 
2.34.1

[RFC 03/15] blockjob: block_job_change_locked(): check job type

[Vladimir Sementsov-Ogievskiy]

User may specify wrong type for the job id. Let's check it.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 blockjob.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/blockjob.c b/blockjob.c
index 8cfbb15543..788cb1e07d 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -319,6 +319,12 @@ void block_job_change_locked(BlockJob *job, JobChangeOptions *opts,
 
     GLOBAL_STATE_CODE();
 
+    if (job_type(&job->job) != opts->type) {
+        error_setg(errp, "Job '%s' is '%s' job, not '%s'", job->job.id,
+                   job_type_str(&job->job), JobType_str(opts->type));
+        return;
+    }
+
     if (job_apply_verb_locked(&job->job, JOB_VERB_CHANGE, errp)) {
         return;
     }
-- 
2.34.1

[RFC 04/15] qapi: block-job-change: make copy-mode parameter optional

[Vladimir Sementsov-Ogievskiy]

We are going to add more parameters to change. We want to make possible
to change only one or any subset of available options. So all the
options should be optional.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 block/mirror.c       | 5 +++++
 qapi/block-core.json | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/block/mirror.c b/block/mirror.c
index a177502e4f..2d0cd22c06 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1265,6 +1265,11 @@ static void mirror_change(BlockJob *job, JobChangeOptions *opts,
 
     GLOBAL_STATE_CODE();
 
+    if (!change_opts->has_copy_mode) {
+        /* Nothing to do */
+        return;
+    }
+
     if (qatomic_read(&s->copy_mode) == change_opts->copy_mode) {
         return;
     }
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 67dd0ef038..6041e7bd8f 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3071,7 +3071,7 @@
 # Since: 8.2
 ##
 { 'struct': 'JobChangeOptionsMirror',
-  'data': { 'copy-mode' : 'MirrorCopyMode' } }
+  'data': { '*copy-mode' : 'MirrorCopyMode' } }
 
 ##
 # @JobChangeOptions:
-- 
2.34.1

[RFC 05/15] qapi: JobChangeOptions: make type member optional and deprecated

[Vladimir Sementsov-Ogievskiy]

Now QEMU can understand type directly from the job itself, type is
redundant.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 blockjob.c                           |  2 +-
 docs/about/deprecated.rst            |  6 ++++++
 job-qmp.c                            | 17 +++++++++++++++++
 qapi/block-core.json                 | 10 ++++++++--
 stubs/meson.build                    |  1 +
 stubs/qapi-jobchangeoptions-mapper.c |  8 ++++++++
 6 files changed, 41 insertions(+), 3 deletions(-)
 create mode 100644 stubs/qapi-jobchangeoptions-mapper.c

diff --git a/blockjob.c b/blockjob.c
index 788cb1e07d..33c40e7d25 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -319,7 +319,7 @@ void block_job_change_locked(BlockJob *job, JobChangeOptions *opts,
 
     GLOBAL_STATE_CODE();
 
-    if (job_type(&job->job) != opts->type) {
+    if (opts->has_type && job_type(&job->job) != opts->type) {
         error_setg(errp, "Job '%s' is '%s' job, not '%s'", job->job.id,
                    job_type_str(&job->job), JobType_str(opts->type));
         return;
diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index dfd681cd02..64981e5e4a 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -142,6 +142,12 @@ accepted incorrect commands will return an error. Users should make sure that
 all arguments passed to ``device_add`` are consistent with the documented
 property types.
 
+
+``block-job-change`` argument ``type`` (since 9.1)
+''''''''''''''''''''''''''''''''''''''''''''''''''
+
+QEMU can get job type from the job itself (by @id), @type field is redundant.
+
 QEMU Machine Protocol (QMP) events
 ----------------------------------
 
diff --git a/job-qmp.c b/job-qmp.c
index 9e26fa899f..c486df9579 100644
--- a/job-qmp.c
+++ b/job-qmp.c
@@ -26,6 +26,8 @@
 #include "qemu/osdep.h"
 #include "qemu/job.h"
 #include "qapi/qapi-commands-job.h"
+#include "qapi/qapi-types-block-core.h"
+#include "qapi/qapi-visit-block-core.h"
 #include "qapi/error.h"
 #include "trace/trace-root.h"
 
@@ -186,3 +188,18 @@ JobInfoList *qmp_query_jobs(Error **errp)
 
     return head;
 }
+
+bool JobChangeOptions_mapper(JobChangeOptions *opts, JobType *out, Error **errp)
+{
+    Job *job;
+
+    JOB_LOCK_GUARD();
+
+    job = find_job_locked(opts->id, errp);
+    if (!job) {
+        return false;
+    }
+
+    *out = job_type(job);
+    return true;
+}
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 6041e7bd8f..3d890dfcd0 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3082,11 +3082,17 @@
 #
 # @type: The job type
 #
+# Features:
+#
+# @deprecated: Members @type is deprecated. Qemu can get type from
+#     the job itself, @type is redundant.
+#
 # Since: 8.2
 ##
 { 'union': 'JobChangeOptions',
-  'base': { 'id': 'str', 'type': 'JobType' },
-  'discriminator': 'type',
+  'base': { 'id': 'str',
+            '*type': { 'type': 'JobType', 'features': ['deprecated'] } },
+  'discriminator': 'JobType',
   'data': { 'mirror': 'JobChangeOptionsMirror' } }
 
 ##
diff --git a/stubs/meson.build b/stubs/meson.build
index 0bf25e6ca5..e480400a6c 100644
--- a/stubs/meson.build
+++ b/stubs/meson.build
@@ -31,6 +31,7 @@ stub_ss.add(files('module-opts.c'))
 stub_ss.add(files('monitor.c'))
 stub_ss.add(files('monitor-core.c'))
 stub_ss.add(files('physmem.c'))
+stub_ss.add(files('qapi-jobchangeoptions-mapper.c'))
 stub_ss.add(files('qemu-timer-notify-cb.c'))
 stub_ss.add(files('memory_device.c'))
 stub_ss.add(files('qmp-command-available.c'))
diff --git a/stubs/qapi-jobchangeoptions-mapper.c b/stubs/qapi-jobchangeoptions-mapper.c
new file mode 100644
index 0000000000..e4acfd91b3
--- /dev/null
+++ b/stubs/qapi-jobchangeoptions-mapper.c
@@ -0,0 +1,8 @@
+#include "qemu/osdep.h"
+#include "qapi/qapi-visit-block-core.h"
+#include "qapi/qapi-types-job.h"
+
+bool JobChangeOptions_mapper(JobChangeOptions *opts, JobType *out, Error **errp)
+{
+    g_assert_not_reached();
+}
-- 
2.34.1

[RFC 06/15] blockjob: move change action implementation to job from block-job

[Vladimir Sementsov-Ogievskiy]

Like for other block-job-* APIs we want have the actual functionality
in job layer and make block-job-change to be a deprecated duplication
of job-change in the following commit.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 block/mirror.c               |  7 +++----
 blockdev.c                   |  2 +-
 blockjob.c                   | 26 --------------------------
 include/block/blockjob.h     | 11 -----------
 include/block/blockjob_int.h |  7 -------
 include/qemu/job.h           | 12 ++++++++++++
 job-qmp.c                    |  1 +
 job.c                        | 23 +++++++++++++++++++++++
 8 files changed, 40 insertions(+), 49 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index 2d0cd22c06..e670d0dd4f 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1251,10 +1251,9 @@ static bool commit_active_cancel(Job *job, bool force)
     return force || !job_is_ready(job);
 }
 
-static void mirror_change(BlockJob *job, JobChangeOptions *opts,
-                          Error **errp)
+static void mirror_change(Job *job, JobChangeOptions *opts, Error **errp)
 {
-    MirrorBlockJob *s = container_of(job, MirrorBlockJob, common);
+    MirrorBlockJob *s = container_of(job, MirrorBlockJob, common.job);
     JobChangeOptionsMirror *change_opts = &opts->u.mirror;
     MirrorCopyMode current;
 
@@ -1310,9 +1309,9 @@ static const BlockJobDriver mirror_job_driver = {
         .pause                  = mirror_pause,
         .complete               = mirror_complete,
         .cancel                 = mirror_cancel,
+        .change                 = mirror_change,
     },
     .drained_poll           = mirror_drained_poll,
-    .change                 = mirror_change,
     .query                  = mirror_query,
 };
 
diff --git a/blockdev.c b/blockdev.c
index 7881f6e5a6..7e13213040 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3256,7 +3256,7 @@ void qmp_block_job_change(JobChangeOptions *opts, Error **errp)
         return;
     }
 
-    block_job_change_locked(job, opts, errp);
+    job_change_locked(&job->job, opts, errp);
 }
 
 void qmp_change_backing_file(const char *device,
diff --git a/blockjob.c b/blockjob.c
index 33c40e7d25..2769722b37 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -312,32 +312,6 @@ static bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)
     return block_job_set_speed_locked(job, speed, errp);
 }
 
-void block_job_change_locked(BlockJob *job, JobChangeOptions *opts,
-                             Error **errp)
-{
-    const BlockJobDriver *drv = block_job_driver(job);
-
-    GLOBAL_STATE_CODE();
-
-    if (opts->has_type && job_type(&job->job) != opts->type) {
-        error_setg(errp, "Job '%s' is '%s' job, not '%s'", job->job.id,
-                   job_type_str(&job->job), JobType_str(opts->type));
-        return;
-    }
-
-    if (job_apply_verb_locked(&job->job, JOB_VERB_CHANGE, errp)) {
-        return;
-    }
-
-    if (drv->change) {
-        job_unlock();
-        drv->change(job, opts, errp);
-        job_lock();
-    } else {
-        error_setg(errp, "Job type does not support change");
-    }
-}
-
 void block_job_ratelimit_processed_bytes(BlockJob *job, uint64_t n)
 {
     IO_CODE();
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 5dd1b08909..72e849a140 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -173,17 +173,6 @@ bool block_job_has_bdrv(BlockJob *job, BlockDriverState *bs);
  */
 bool block_job_set_speed_locked(BlockJob *job, int64_t speed, Error **errp);
 
-/**
- * block_job_change_locked:
- * @job: The job to change.
- * @opts: The new options.
- * @errp: Error object.
- *
- * Change the job according to opts.
- */
-void block_job_change_locked(BlockJob *job, JobChangeOptions *opts,
-                             Error **errp);
-
 /**
  * block_job_query_locked:
  * @job: The job to get information about.
diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
index d9c3b911d0..58bc7a5cea 100644
--- a/include/block/blockjob_int.h
+++ b/include/block/blockjob_int.h
@@ -68,13 +68,6 @@ struct BlockJobDriver {
 
     void (*set_speed)(BlockJob *job, int64_t speed);
 
-    /*
-     * Change the @job's options according to @opts.
-     *
-     * Note that this can already be called before the job coroutine is running.
-     */
-    void (*change)(BlockJob *job, JobChangeOptions *opts, Error **errp);
-
     /*
      * Query information specific to this kind of block job.
      */
diff --git a/include/qemu/job.h b/include/qemu/job.h
index 9ea98b5927..d44cdb0cc8 100644
--- a/include/qemu/job.h
+++ b/include/qemu/job.h
@@ -27,6 +27,7 @@
 #define JOB_H
 
 #include "qapi/qapi-types-job.h"
+#include "qapi/qapi-types-block-core.h"
 #include "qemu/queue.h"
 #include "qemu/progress_meter.h"
 #include "qemu/coroutine.h"
@@ -307,6 +308,12 @@ struct JobDriver {
      */
     bool (*cancel)(Job *job, bool force);
 
+    /**
+     * Change the @job's options according to @opts.
+     *
+     * Note that this can already be called before the job coroutine is running.
+     */
+    void (*change)(Job *job, JobChangeOptions *opts, Error **errp);
 
     /**
      * Called when the job is freed.
@@ -705,6 +712,11 @@ void job_finalize_locked(Job *job, Error **errp);
  */
 void job_dismiss_locked(Job **job, Error **errp);
 
+/**
+ * Change the job according to opts.
+ */
+void job_change_locked(Job *job, JobChangeOptions *opts, Error **errp);
+
 /**
  * Synchronously finishes the given @job. If @finish is given, it is called to
  * trigger completion or cancellation of the job.
diff --git a/job-qmp.c b/job-qmp.c
index c486df9579..abe9b59487 100644
--- a/job-qmp.c
+++ b/job-qmp.c
@@ -28,6 +28,7 @@
 #include "qapi/qapi-commands-job.h"
 #include "qapi/qapi-types-block-core.h"
 #include "qapi/qapi-visit-block-core.h"
+#include "qapi/qapi-commands-block-core.h"
 #include "qapi/error.h"
 #include "trace/trace-root.h"
 
diff --git a/job.c b/job.c
index 660ce22c56..69630852dc 100644
--- a/job.c
+++ b/job.c
@@ -1262,3 +1262,26 @@ int job_finish_sync_locked(Job *job,
     job_unref_locked(job);
     return ret;
 }
+
+void job_change_locked(Job *job, JobChangeOptions *opts, Error **errp)
+{
+    GLOBAL_STATE_CODE();
+
+    if (opts->has_type && job_type(job) != opts->type) {
+        error_setg(errp, "Job '%s' is '%s' job, not '%s'", job->id,
+                   job_type_str(job), JobType_str(opts->type));
+        return;
+    }
+
+    if (job_apply_verb_locked(job, JOB_VERB_CHANGE, errp)) {
+        return;
+    }
+
+    if (job->driver->change) {
+        job_unlock();
+        job->driver->change(job, opts, errp);
+        job_lock();
+    } else {
+        error_setg(errp, "Job type does not support change");
+    }
+}
-- 
2.34.1

[RFC 07/15] qapi: add job-change

[Vladimir Sementsov-Ogievskiy]

Add a new-style command job-change, doing same thing as
block-job-change. The aim is finally deprecate block-job-* APIs and
move to job-* APIs.

We add a new command to qapi/block-core.json, not to
qapi/job.json to avoid resolving json file including loops for now.
This all would be a lot simple to refactor when we finally drop
deprecated block-job-* APIs.

@type argument of the new command immediately becomes deprecated.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 docs/about/deprecated.rst |  4 ++--
 job-qmp.c                 | 14 ++++++++++++++
 qapi/block-core.json      | 10 ++++++++++
 3 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 64981e5e4a..5ff98ef95f 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -143,8 +143,8 @@ all arguments passed to ``device_add`` are consistent with the documented
 property types.
 
 
-``block-job-change`` argument ``type`` (since 9.1)
-''''''''''''''''''''''''''''''''''''''''''''''''''
+``block-job-change`` and ``job-change``  argument ``type`` (since 9.1)
+''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
 
 QEMU can get job type from the job itself (by @id), @type field is redundant.
 
diff --git a/job-qmp.c b/job-qmp.c
index abe9b59487..011a8736ea 100644
--- a/job-qmp.c
+++ b/job-qmp.c
@@ -141,6 +141,20 @@ void qmp_job_dismiss(const char *id, Error **errp)
     job_dismiss_locked(&job, errp);
 }
 
+void qmp_job_change(JobChangeOptions *opts, Error **errp)
+{
+    Job *job;
+
+    JOB_LOCK_GUARD();
+    job = find_job_locked(opts->id, errp);
+
+    if (!job) {
+        return;
+    }
+
+    job_change_locked(job, opts, errp);
+}
+
 /* Called with job_mutex held. */
 static JobInfo *job_query_single_locked(Job *job, Error **errp)
 {
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 3d890dfcd0..f5cefa441b 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3105,6 +3105,16 @@
 { 'command': 'block-job-change',
   'data': 'JobChangeOptions', 'boxed': true }
 
+##
+# @job-change:
+#
+# Change the block job's options.
+#
+# Since: 9.1
+##
+{ 'command': 'job-change',
+  'data': 'JobChangeOptions', 'boxed': true }
+
 ##
 # @BlockdevDiscardOptions:
 #
-- 
2.34.1

[RFC 08/15] qapi: job-change: support speed parameter

[Vladimir Sementsov-Ogievskiy]

Allow change job speed through job-change command. Old
block-job-set-speed would be deprecated soon.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 block/backup.c           |  8 ++++++
 block/commit.c           | 10 ++++++-
 block/mirror.c           | 60 ++++++++++++++++++++++++----------------
 block/stream.c           |  8 ++++++
 blockjob.c               | 13 +++++++++
 include/block/blockjob.h |  7 +++++
 include/qemu/job.h       |  2 +-
 qapi/block-core.json     | 23 +++++++++++++--
 8 files changed, 103 insertions(+), 28 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index ec29d6b810..bf086dc5f9 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -336,6 +336,13 @@ static bool backup_cancel(Job *job, bool force)
     return true;
 }
 
+static bool backup_change(Job *job, JobChangeOptions *opts, Error **errp)
+{
+    BlockJob *bjob = container_of(job, BlockJob, job);
+
+    return block_job_change(bjob, &opts->u.backup, errp);
+}
+
 static const BlockJobDriver backup_job_driver = {
     .job_driver = {
         .instance_size          = sizeof(BackupBlockJob),
@@ -348,6 +355,7 @@ static const BlockJobDriver backup_job_driver = {
         .clean                  = backup_clean,
         .pause                  = backup_pause,
         .cancel                 = backup_cancel,
+        .change                 = backup_change,
     },
     .set_speed = backup_set_speed,
 };
diff --git a/block/commit.c b/block/commit.c
index 7c3fdcb0ca..ccb6097679 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -204,6 +204,13 @@ static int coroutine_fn commit_run(Job *job, Error **errp)
     return 0;
 }
 
+static bool commit_change(Job *job, JobChangeOptions *opts, Error **errp)
+{
+    BlockJob *bjob = container_of(job, BlockJob, job);
+
+    return block_job_change(bjob, &opts->u.commit, errp);
+}
+
 static const BlockJobDriver commit_job_driver = {
     .job_driver = {
         .instance_size = sizeof(CommitBlockJob),
@@ -213,7 +220,8 @@ static const BlockJobDriver commit_job_driver = {
         .run           = commit_run,
         .prepare       = commit_prepare,
         .abort         = commit_abort,
-        .clean         = commit_clean
+        .clean         = commit_clean,
+        .change        = commit_change,
     },
 };
 
diff --git a/block/mirror.c b/block/mirror.c
index e670d0dd4f..f474b87079 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1251,41 +1251,45 @@ static bool commit_active_cancel(Job *job, bool force)
     return force || !job_is_ready(job);
 }
 
-static void mirror_change(Job *job, JobChangeOptions *opts, Error **errp)
+static bool mirror_change(Job *job, JobChangeOptions *opts, Error **errp)
 {
+    BlockJob *bjob = container_of(job, BlockJob, job);
     MirrorBlockJob *s = container_of(job, MirrorBlockJob, common.job);
     JobChangeOptionsMirror *change_opts = &opts->u.mirror;
-    MirrorCopyMode current;
-
-    /*
-     * The implementation relies on the fact that copy_mode is only written
-     * under the BQL. Otherwise, further synchronization would be required.
-     */
+    MirrorCopyMode old_mode;
 
     GLOBAL_STATE_CODE();
 
-    if (!change_opts->has_copy_mode) {
-        /* Nothing to do */
-        return;
-    }
+    if (change_opts->has_copy_mode) {
+        old_mode = qatomic_read(&s->copy_mode);
 
-    if (qatomic_read(&s->copy_mode) == change_opts->copy_mode) {
-        return;
-    }
+        if (old_mode != change_opts->copy_mode) {
+            if (change_opts->copy_mode != MIRROR_COPY_MODE_WRITE_BLOCKING) {
+                error_setg(errp, "Change to copy mode '%s' is not implemented",
+                           MirrorCopyMode_str(change_opts->copy_mode));
+                return false;
+            }
 
-    if (change_opts->copy_mode != MIRROR_COPY_MODE_WRITE_BLOCKING) {
-        error_setg(errp, "Change to copy mode '%s' is not implemented",
-                   MirrorCopyMode_str(change_opts->copy_mode));
-        return;
+            if (old_mode != MIRROR_COPY_MODE_BACKGROUND) {
+                error_setg(errp, "Expected current copy mode '%s', got '%s'",
+                           MirrorCopyMode_str(MIRROR_COPY_MODE_BACKGROUND),
+                           MirrorCopyMode_str(old_mode));
+                return false;
+            }
+        }
     }
 
-    current = qatomic_cmpxchg(&s->copy_mode, MIRROR_COPY_MODE_BACKGROUND,
-                              change_opts->copy_mode);
-    if (current != MIRROR_COPY_MODE_BACKGROUND) {
-        error_setg(errp, "Expected current copy mode '%s', got '%s'",
-                   MirrorCopyMode_str(MIRROR_COPY_MODE_BACKGROUND),
-                   MirrorCopyMode_str(current));
+    if (!block_job_change(bjob, qapi_JobChangeOptionsMirror_base(change_opts),
+                          errp))
+    {
+        return false;
     }
+
+    old_mode = qatomic_cmpxchg(&s->copy_mode, MIRROR_COPY_MODE_BACKGROUND,
+                               change_opts->copy_mode);
+    assert(old_mode == MIRROR_COPY_MODE_BACKGROUND);
+
+    return true;
 }
 
 static void mirror_query(BlockJob *job, BlockJobInfo *info)
@@ -1315,6 +1319,13 @@ static const BlockJobDriver mirror_job_driver = {
     .query                  = mirror_query,
 };
 
+static bool commit_active_change(Job *job, JobChangeOptions *opts, Error **errp)
+{
+    BlockJob *bjob = container_of(job, BlockJob, job);
+
+    return block_job_change(bjob, &opts->u.commit, errp);
+}
+
 static const BlockJobDriver commit_active_job_driver = {
     .job_driver = {
         .instance_size          = sizeof(MirrorBlockJob),
@@ -1327,6 +1338,7 @@ static const BlockJobDriver commit_active_job_driver = {
         .pause                  = mirror_pause,
         .complete               = mirror_complete,
         .cancel                 = commit_active_cancel,
+        .change                 = commit_active_change,
     },
     .drained_poll           = mirror_drained_poll,
 };
diff --git a/block/stream.c b/block/stream.c
index 7031eef12b..34f4588537 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -239,6 +239,13 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
     return error;
 }
 
+static bool stream_change(Job *job, JobChangeOptions *opts, Error **errp)
+{
+    BlockJob *bjob = container_of(job, BlockJob, job);
+
+    return block_job_change(bjob, &opts->u.stream, errp);
+}
+
 static const BlockJobDriver stream_job_driver = {
     .job_driver = {
         .instance_size = sizeof(StreamBlockJob),
@@ -248,6 +255,7 @@ static const BlockJobDriver stream_job_driver = {
         .prepare       = stream_prepare,
         .clean         = stream_clean,
         .user_resume   = block_job_user_resume,
+        .change        = stream_change,
     },
 };
 
diff --git a/blockjob.c b/blockjob.c
index 2769722b37..d3cd4f4fbf 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -312,6 +312,19 @@ static bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)
     return block_job_set_speed_locked(job, speed, errp);
 }
 
+bool block_job_change(BlockJob *job, JobChangeOptionsBlockJob *opts,
+                      Error **errp)
+{
+    GLOBAL_STATE_CODE();
+
+    if (!opts->has_speed) {
+        /* Nothing to do */
+        return true;
+    }
+
+    return block_job_set_speed(job, opts->speed, errp);
+}
+
 void block_job_ratelimit_processed_bytes(BlockJob *job, uint64_t n)
 {
     IO_CODE();
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 72e849a140..fe433c8d35 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -224,4 +224,11 @@ bool block_job_is_internal(BlockJob *job);
  */
 const BlockJobDriver *block_job_driver(BlockJob *job);
 
+/**
+ * Common part of .change handler for block-jobs.
+ * Applies changes described in opts to the job.
+ */
+bool block_job_change(BlockJob *job, JobChangeOptionsBlockJob *opts,
+                      Error **errp);
+
 #endif
diff --git a/include/qemu/job.h b/include/qemu/job.h
index d44cdb0cc8..1c9da74a0c 100644
--- a/include/qemu/job.h
+++ b/include/qemu/job.h
@@ -313,7 +313,7 @@ struct JobDriver {
      *
      * Note that this can already be called before the job coroutine is running.
      */
-    void (*change)(Job *job, JobChangeOptions *opts, Error **errp);
+    bool (*change)(Job *job, JobChangeOptions *opts, Error **errp);
 
     /**
      * Called when the job is freed.
diff --git a/qapi/block-core.json b/qapi/block-core.json
index f5cefa441b..93f96e747e 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3062,6 +3062,17 @@
 { 'command': 'block-job-finalize', 'data': { 'id': 'str' },
   'allow-preconfig': true }
 
+##
+# @JobChangeOptionsBlockJob:
+#
+# @speed: Change job speed (in bytes per second).  Supported for
+#     for backup, mirror, commit and stream jobs.
+#
+# Since: 9.1
+##
+{ 'struct': 'JobChangeOptionsBlockJob',
+  'data': { '*speed' : 'uint64' } }
+
 ##
 # @JobChangeOptionsMirror:
 #
@@ -3071,12 +3082,17 @@
 # Since: 8.2
 ##
 { 'struct': 'JobChangeOptionsMirror',
+  'base': 'JobChangeOptionsBlockJob',
   'data': { '*copy-mode' : 'MirrorCopyMode' } }
 
+
 ##
 # @JobChangeOptions:
 #
-# Block job options that can be changed after job creation.
+# Job options that can be changed after job creation. When option is
+# not specified the corresponding job parameter remains unchanged.
+# The change is transactional: on success all changes are applied
+# successfully, on failure nothing is changed.
 #
 # @id: The job identifier
 #
@@ -3093,7 +3109,10 @@
   'base': { 'id': 'str',
             '*type': { 'type': 'JobType', 'features': ['deprecated'] } },
   'discriminator': 'JobType',
-  'data': { 'mirror': 'JobChangeOptionsMirror' } }
+  'data': { 'mirror': 'JobChangeOptionsMirror',
+            'backup': 'JobChangeOptionsBlockJob',
+            'stream': 'JobChangeOptionsBlockJob',
+            'commit': 'JobChangeOptionsBlockJob' } }
 
 ##
 # @block-job-change:
-- 
2.34.1

[RFC 09/15] qapi: job-complete: introduce no-block-replace option for mirror

[Vladimir Sementsov-Ogievskiy]

That's an alternative to block-job-cancel(hard=false) behavior for
mirror, which exactly is: complete the job, wait for in-flight
requests, but don't do any block graph modifications.

Why:

1. We move to deprecating block-job-* APIs and use job-* APIs instead.
   So we need to port somehow the functionality to job- API.

2. The specified behavior was also a kind of "quirk". It's strange to
   "cancel" something in a normal success path of user scenario. This
   block-job-cancel(hard=false) results in BLOCK_JOB_COMPLETE event, so
   definitely it's just another mode of "complete", not "cancel".

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 block/mirror.c                       |  8 +++++---
 include/qemu/job.h                   |  8 +++++++-
 job-qmp.c                            | 20 +++++++++++++++-----
 job.c                                | 12 ++++++++++--
 qapi/job.json                        | 28 +++++++++++++++++++++++++---
 stubs/qapi-jobchangeoptions-mapper.c |  5 +++++
 tests/unit/test-bdrv-drain.c         |  2 +-
 tests/unit/test-block-iothread.c     |  2 +-
 tests/unit/test-blockjob.c           |  2 +-
 9 files changed, 70 insertions(+), 17 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index f474b87079..e95c54fbc6 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -69,6 +69,7 @@ typedef struct MirrorBlockJob {
      */
     bool actively_synced;
     bool should_complete;
+    bool no_block_replace;
     int64_t granularity;
     size_t buf_size;
     int64_t bdev_length;
@@ -740,7 +741,7 @@ static int mirror_exit_common(Job *job)
     }
     bdrv_graph_rdunlock_main_loop();
 
-    if (s->should_complete && !abort) {
+    if (s->should_complete && !abort && !s->no_block_replace) {
         BlockDriverState *to_replace = s->to_replace ?: src;
         bool ro = bdrv_is_read_only(to_replace);
 
@@ -1167,7 +1168,7 @@ immediate_exit:
     return ret;
 }
 
-static void mirror_complete(Job *job, Error **errp)
+static void mirror_complete(Job *job, JobComplete *opts, Error **errp)
 {
     MirrorBlockJob *s = container_of(job, MirrorBlockJob, common.job);
 
@@ -1178,7 +1179,7 @@ static void mirror_complete(Job *job, Error **errp)
     }
 
     /* block all operations on to_replace bs */
-    if (s->replaces) {
+    if (s->replaces && !opts->u.mirror.no_block_replace) {
         s->to_replace = bdrv_find_node(s->replaces);
         if (!s->to_replace) {
             error_setg(errp, "Node name '%s' not found", s->replaces);
@@ -1193,6 +1194,7 @@ static void mirror_complete(Job *job, Error **errp)
     }
 
     s->should_complete = true;
+    s->no_block_replace = opts->u.mirror.no_block_replace;
 
     /* If the job is paused, it will be re-entered when it is resumed */
     WITH_JOB_LOCK_GUARD() {
diff --git a/include/qemu/job.h b/include/qemu/job.h
index 1c9da74a0c..eee1d5b50f 100644
--- a/include/qemu/job.h
+++ b/include/qemu/job.h
@@ -254,7 +254,7 @@ struct JobDriver {
      * Optional callback for job types whose completion must be triggered
      * manually.
      */
-    void (*complete)(Job *job, Error **errp);
+    void (*complete)(Job *job, JobComplete *opts, Error **errp);
 
     /**
      * If the callback is not NULL, prepare will be invoked when all the jobs
@@ -634,6 +634,12 @@ void job_early_fail(Job *job);
  */
 void job_transition_to_ready(Job *job);
 
+/**
+ * Asynchronously complete the specified @job.
+ * Called with job lock held, but might release it temporarily.
+ */
+void job_complete_opts_locked(Job *job, JobComplete *opts, Error **errp);
+
 /**
  * Asynchronously complete the specified @job.
  * Called with job lock held, but might release it temporarily.
diff --git a/job-qmp.c b/job-qmp.c
index 011a8736ea..c048e03d9f 100644
--- a/job-qmp.c
+++ b/job-qmp.c
@@ -93,19 +93,19 @@ void qmp_job_resume(const char *id, Error **errp)
     job_user_resume_locked(job, errp);
 }
 
-void qmp_job_complete(const char *id, Error **errp)
+void qmp_job_complete(JobComplete *opts, Error **errp)
 {
     Job *job;
 
     JOB_LOCK_GUARD();
-    job = find_job_locked(id, errp);
+    job = find_job_locked(opts->id, errp);
 
     if (!job) {
         return;
     }
 
     trace_qmp_job_complete(job);
-    job_complete_locked(job, errp);
+    job_complete_opts_locked(job, opts, errp);
 }
 
 void qmp_job_finalize(const char *id, Error **errp)
@@ -204,13 +204,13 @@ JobInfoList *qmp_query_jobs(Error **errp)
     return head;
 }
 
-bool JobChangeOptions_mapper(JobChangeOptions *opts, JobType *out, Error **errp)
+static bool job_mapper(const char *id, JobType *out, Error **errp)
 {
     Job *job;
 
     JOB_LOCK_GUARD();
 
-    job = find_job_locked(opts->id, errp);
+    job = find_job_locked(id, errp);
     if (!job) {
         return false;
     }
@@ -218,3 +218,13 @@ bool JobChangeOptions_mapper(JobChangeOptions *opts, JobType *out, Error **errp)
     *out = job_type(job);
     return true;
 }
+
+bool JobChangeOptions_mapper(JobChangeOptions *opts, JobType *out, Error **errp)
+{
+    return job_mapper(opts->id, out, errp);
+}
+
+bool JobComplete_mapper(JobComplete *opts, JobType *out, Error **errp)
+{
+    return job_mapper(opts->id, out, errp);
+}
diff --git a/job.c b/job.c
index 69630852dc..3726ba2c9e 100644
--- a/job.c
+++ b/job.c
@@ -1214,11 +1214,14 @@ int job_complete_sync_locked(Job *job, Error **errp)
     return job_finish_sync_locked(job, job_complete_locked, errp);
 }
 
-void job_complete_locked(Job *job, Error **errp)
+void job_complete_opts_locked(Job *job, JobComplete *opts, Error **errp)
 {
+    JobComplete local_opts = {};
+
     /* Should not be reachable via external interface for internal jobs */
     assert(job->id);
     GLOBAL_STATE_CODE();
+
     if (job_apply_verb_locked(job, JOB_VERB_COMPLETE, errp)) {
         return;
     }
@@ -1229,10 +1232,15 @@ void job_complete_locked(Job *job, Error **errp)
     }
 
     job_unlock();
-    job->driver->complete(job, errp);
+    job->driver->complete(job, opts ?: &local_opts, errp);
     job_lock();
 }
 
+void job_complete_locked(Job *job, Error **errp)
+{
+    job_complete_opts_locked(job, NULL, errp);
+}
+
 int job_finish_sync_locked(Job *job,
                            void (*finish)(Job *, Error **errp),
                            Error **errp)
diff --git a/qapi/job.json b/qapi/job.json
index b3957207a4..2f1b839cfc 100644
--- a/qapi/job.json
+++ b/qapi/job.json
@@ -182,15 +182,37 @@
 { 'command': 'job-cancel', 'data': { 'id': 'str' } }
 
 ##
-# @job-complete:
+# @JobCompleteMirror:
 #
-# Manually trigger completion of an active job in the READY state.
+# @no-block-replace: Supported only for mirror job. If true, alter
+#     the mirror job completion behavior so that final switch to
+#     target block node is not done. Since 9.1
+#
+# Since: 9.1
+##
+{ 'struct': 'JobCompleteMirror',
+  'data': { '*no-block-replace': 'bool' } }
+
+##
+# @JobComplete:
 #
 # @id: The job identifier.
 #
 # Since: 3.0
 ##
-{ 'command': 'job-complete', 'data': { 'id': 'str' } }
+{ 'union': 'JobComplete',
+  'base': { 'id': 'str' },
+  'discriminator': 'JobType',
+  'data': { 'mirror': 'JobCompleteMirror' } }
+
+##
+# @job-complete:
+#
+# Manually trigger completion of an active job in the READY state.
+#
+# Since: 3.0
+##
+{ 'command': 'job-complete', 'data': 'JobComplete', 'boxed': true }
 
 ##
 # @job-dismiss:
diff --git a/stubs/qapi-jobchangeoptions-mapper.c b/stubs/qapi-jobchangeoptions-mapper.c
index e4acfd91b3..8031a5317c 100644
--- a/stubs/qapi-jobchangeoptions-mapper.c
+++ b/stubs/qapi-jobchangeoptions-mapper.c
@@ -6,3 +6,8 @@ bool JobChangeOptions_mapper(JobChangeOptions *opts, JobType *out, Error **errp)
 {
     g_assert_not_reached();
 }
+
+bool JobComplete_mapper(JobComplete *opts, JobType *out, Error **errp)
+{
+    g_assert_not_reached();
+}
diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c
index 666880472b..933d3dd990 100644
--- a/tests/unit/test-bdrv-drain.c
+++ b/tests/unit/test-bdrv-drain.c
@@ -682,7 +682,7 @@ static int coroutine_fn test_job_run(Job *job, Error **errp)
     return s->run_ret;
 }
 
-static void test_job_complete(Job *job, Error **errp)
+static void test_job_complete(Job *job, JobComplete *opts, Error **errp)
 {
     TestBlockJob *s = container_of(job, TestBlockJob, common.job);
     s->should_complete = true;
diff --git a/tests/unit/test-block-iothread.c b/tests/unit/test-block-iothread.c
index 3766d5de6b..dce33a8b9b 100644
--- a/tests/unit/test-block-iothread.c
+++ b/tests/unit/test-block-iothread.c
@@ -529,7 +529,7 @@ static int coroutine_fn test_job_run(Job *job, Error **errp)
     return 0;
 }
 
-static void test_job_complete(Job *job, Error **errp)
+static void test_job_complete(Job *job, JobComplete *opts, Error **errp)
 {
     TestBlockJob *s = container_of(job, TestBlockJob, common.job);
     s->should_complete = true;
diff --git a/tests/unit/test-blockjob.c b/tests/unit/test-blockjob.c
index fe3e0d2d38..027a2dd886 100644
--- a/tests/unit/test-blockjob.c
+++ b/tests/unit/test-blockjob.c
@@ -165,7 +165,7 @@ typedef struct CancelJob {
     bool should_complete;
 } CancelJob;
 
-static void cancel_job_complete(Job *job, Error **errp)
+static void cancel_job_complete(Job *job, JobComplete *opts, Error **errp)
 {
     CancelJob *s = container_of(job, CancelJob, common.job);
     s->should_complete = true;
-- 
2.34.1

[RFC 10/15] qapi: query-jobs: add information specific for job type

[Vladimir Sementsov-Ogievskiy]

Duplicate the feature from query-block-jobs. It's a step to finally
deprecate query-block-jobs command and move to query-jobs.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 block/mirror.c     | 14 ++++++++++++--
 include/qemu/job.h |  5 +++++
 job-qmp.c          |  6 ++++++
 qapi/job.json      | 22 +++++++++++++++++++---
 4 files changed, 42 insertions(+), 5 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index e95c54fbc6..96dcbbc3e8 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1294,7 +1294,7 @@ static bool mirror_change(Job *job, JobChangeOptions *opts, Error **errp)
     return true;
 }
 
-static void mirror_query(BlockJob *job, BlockJobInfo *info)
+static void mirror_query_old(BlockJob *job, BlockJobInfo *info)
 {
     MirrorBlockJob *s = container_of(job, MirrorBlockJob, common);
 
@@ -1303,6 +1303,15 @@ static void mirror_query(BlockJob *job, BlockJobInfo *info)
     };
 }
 
+static void mirror_query(Job *job, JobInfo *info)
+{
+    MirrorBlockJob *s = container_of(job, MirrorBlockJob, common.job);
+
+    info->u.mirror = (JobInfoMirror) {
+        .actively_synced = qatomic_read(&s->actively_synced),
+    };
+}
+
 static const BlockJobDriver mirror_job_driver = {
     .job_driver = {
         .instance_size          = sizeof(MirrorBlockJob),
@@ -1316,9 +1325,10 @@ static const BlockJobDriver mirror_job_driver = {
         .complete               = mirror_complete,
         .cancel                 = mirror_cancel,
         .change                 = mirror_change,
+        .query                  = mirror_query,
     },
     .drained_poll           = mirror_drained_poll,
-    .query                  = mirror_query,
+    .query                  = mirror_query_old,
 };
 
 static bool commit_active_change(Job *job, JobChangeOptions *opts, Error **errp)
diff --git a/include/qemu/job.h b/include/qemu/job.h
index eee1d5b50f..8a238b8658 100644
--- a/include/qemu/job.h
+++ b/include/qemu/job.h
@@ -315,6 +315,11 @@ struct JobDriver {
      */
     bool (*change)(Job *job, JobChangeOptions *opts, Error **errp);
 
+    /*
+     * Query information specific to this kind of block job.
+     */
+    void (*query)(Job *job, JobInfo *info);
+
     /**
      * Called when the job is freed.
      */
diff --git a/job-qmp.c b/job-qmp.c
index c048e03d9f..9643c5424d 100644
--- a/job-qmp.c
+++ b/job-qmp.c
@@ -177,6 +177,12 @@ static JobInfo *job_query_single_locked(Job *job, Error **errp)
                               g_strdup(error_get_pretty(job->err)) : NULL,
     };
 
+    if (job->driver->query) {
+        job_unlock();
+        job->driver->query(job, info);
+        job_lock();
+    }
+
     return info;
 }
 
diff --git a/qapi/job.json b/qapi/job.json
index 2f1b839cfc..036fec1b57 100644
--- a/qapi/job.json
+++ b/qapi/job.json
@@ -251,6 +251,20 @@
 ##
 { 'command': 'job-finalize', 'data': { 'id': 'str' } }
 
+##
+# @JobInfoMirror:
+#
+# Information specific to mirror block jobs.
+#
+# @actively-synced: Whether the source is actively synced to the
+#     target, i.e. same data and new writes are done synchronously to
+#     both.
+#
+# Since: 9.1
+##
+{ 'struct': 'JobInfoMirror',
+  'data': { 'actively-synced': 'bool' } }
+
 ##
 # @JobInfo:
 #
@@ -281,10 +295,12 @@
 #
 # Since: 3.0
 ##
-{ 'struct': 'JobInfo',
-  'data': { 'id': 'str', 'type': 'JobType', 'status': 'JobStatus',
+{ 'union': 'JobInfo',
+  'base': { 'id': 'str', 'type': 'JobType', 'status': 'JobStatus',
             'current-progress': 'int', 'total-progress': 'int',
-            '*error': 'str' } }
+            '*error': 'str' },
+  'discriminator': 'type',
+  'data': { 'mirror': 'JobInfoMirror' } }
 
 ##
 # @query-jobs:
-- 
2.34.1

[RFC 11/15] job-qmp: job_query_single_locked: add assertion on job ret

[Vladimir Sementsov-Ogievskiy]

job->ret must be always set together with job->err. Let's assert this.
Reproting no-error to the user, when job->err is unset and job->ret is
somehow set would be a bug.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 job-qmp.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/job-qmp.c b/job-qmp.c
index 9643c5424d..3e2172c26a 100644
--- a/job-qmp.c
+++ b/job-qmp.c
@@ -163,6 +163,7 @@ static JobInfo *job_query_single_locked(Job *job, Error **errp)
     uint64_t progress_total;
 
     assert(!job_is_internal(job));
+    assert(!job->err == !job->ret);
     progress_get_snapshot(&job->progress, &progress_current,
                           &progress_total);
 
-- 
2.34.1

[RFC 12/15] qapi: rename BlockDeviceIoStatus to IoStatus

[Vladimir Sementsov-Ogievskiy]

This status is already shared between block-jobs and block-devices, so
structure name is misleading a bit. We also will need to use the
structure both in block-core.json and job.json. So give it more generic
name first.

The commit is made by commands:

    git grep -l BlockDeviceIoStatus | \
        xargs sed -i 's/BlockDeviceIoStatus/IoStatus/g'

    git grep -l BLOCK_DEVICE_IO_STATUS | \
        xargs sed -i 's/BLOCK_DEVICE_IO_STATUS/IO_STATUS/g'

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 block/block-backend.c                       | 14 +++++++-------
 block/mirror.c                              |  4 ++--
 block/monitor/block-hmp-cmds.c              |  4 ++--
 blockjob.c                                  | 10 +++++-----
 include/block/blockjob.h                    |  2 +-
 include/sysemu/block-backend-global-state.h |  2 +-
 qapi/block-core.json                        | 10 +++++-----
 7 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 9c4de79e6b..ec19c50e96 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -65,7 +65,7 @@ struct BlockBackend {
 
     BlockdevOnError on_read_error, on_write_error;
     bool iostatus_enabled;
-    BlockDeviceIoStatus iostatus;
+    IoStatus iostatus;
 
     uint64_t perm;
     uint64_t shared_perm;
@@ -1198,7 +1198,7 @@ void blk_iostatus_enable(BlockBackend *blk)
 {
     GLOBAL_STATE_CODE();
     blk->iostatus_enabled = true;
-    blk->iostatus = BLOCK_DEVICE_IO_STATUS_OK;
+    blk->iostatus = IO_STATUS_OK;
 }
 
 /* The I/O status is only enabled if the drive explicitly
@@ -1212,7 +1212,7 @@ bool blk_iostatus_is_enabled(const BlockBackend *blk)
             blk->on_read_error == BLOCKDEV_ON_ERROR_STOP));
 }
 
-BlockDeviceIoStatus blk_iostatus(const BlockBackend *blk)
+IoStatus blk_iostatus(const BlockBackend *blk)
 {
     GLOBAL_STATE_CODE();
     return blk->iostatus;
@@ -1228,7 +1228,7 @@ void blk_iostatus_reset(BlockBackend *blk)
 {
     GLOBAL_STATE_CODE();
     if (blk_iostatus_is_enabled(blk)) {
-        blk->iostatus = BLOCK_DEVICE_IO_STATUS_OK;
+        blk->iostatus = IO_STATUS_OK;
     }
 }
 
@@ -1236,9 +1236,9 @@ void blk_iostatus_set_err(BlockBackend *blk, int error)
 {
     IO_CODE();
     assert(blk_iostatus_is_enabled(blk));
-    if (blk->iostatus == BLOCK_DEVICE_IO_STATUS_OK) {
-        blk->iostatus = error == ENOSPC ? BLOCK_DEVICE_IO_STATUS_NOSPACE :
-                                          BLOCK_DEVICE_IO_STATUS_FAILED;
+    if (blk->iostatus == IO_STATUS_OK) {
+        blk->iostatus = error == ENOSPC ? IO_STATUS_NOSPACE :
+                                          IO_STATUS_FAILED;
     }
 }
 
diff --git a/block/mirror.c b/block/mirror.c
index 96dcbbc3e8..8e672f3309 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -923,7 +923,7 @@ static int coroutine_fn mirror_run(Job *job, Error **errp)
     MirrorBDSOpaque *mirror_top_opaque = s->mirror_top_bs->opaque;
     BlockDriverState *target_bs = blk_bs(s->target);
     bool need_drain = true;
-    BlockDeviceIoStatus iostatus;
+    IoStatus iostatus;
     int64_t length;
     int64_t target_length;
     BlockDriverInfo bdi;
@@ -1060,7 +1060,7 @@ static int coroutine_fn mirror_run(Job *job, Error **errp)
             iostatus = s->common.iostatus;
         }
         if (delta < BLOCK_JOB_SLICE_TIME &&
-            iostatus == BLOCK_DEVICE_IO_STATUS_OK) {
+            iostatus == IO_STATUS_OK) {
             if (s->in_flight >= MAX_IN_FLIGHT || s->buf_free_count == 0 ||
                 (cnt == 0 && s->in_flight > 0)) {
                 trace_mirror_yield(s, cnt, s->buf_free_count, s->in_flight);
diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index d954bec6f1..64acb9cd6a 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -642,9 +642,9 @@ static void print_block_info(Monitor *mon, BlockInfo *info,
         if (info->qdev) {
             monitor_printf(mon, "    Attached to:      %s\n", info->qdev);
         }
-        if (info->has_io_status && info->io_status != BLOCK_DEVICE_IO_STATUS_OK) {
+        if (info->has_io_status && info->io_status != IO_STATUS_OK) {
             monitor_printf(mon, "    I/O status:       %s\n",
-                           BlockDeviceIoStatus_str(info->io_status));
+                           IoStatus_str(info->io_status));
         }
 
         if (info->removable) {
diff --git a/blockjob.c b/blockjob.c
index d3cd4f4fbf..de1dd03b2d 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -394,9 +394,9 @@ BlockJobInfo *block_job_query_locked(BlockJob *job, Error **errp)
 /* Called with job lock held */
 static void block_job_iostatus_set_err_locked(BlockJob *job, int error)
 {
-    if (job->iostatus == BLOCK_DEVICE_IO_STATUS_OK) {
-        job->iostatus = error == ENOSPC ? BLOCK_DEVICE_IO_STATUS_NOSPACE :
-                                          BLOCK_DEVICE_IO_STATUS_FAILED;
+    if (job->iostatus == IO_STATUS_OK) {
+        job->iostatus = error == ENOSPC ? IO_STATUS_NOSPACE :
+                                          IO_STATUS_FAILED;
     }
 }
 
@@ -550,11 +550,11 @@ fail:
 void block_job_iostatus_reset_locked(BlockJob *job)
 {
     GLOBAL_STATE_CODE();
-    if (job->iostatus == BLOCK_DEVICE_IO_STATUS_OK) {
+    if (job->iostatus == IO_STATUS_OK) {
         return;
     }
     assert(job->job.user_paused && job->job.pause_count > 0);
-    job->iostatus = BLOCK_DEVICE_IO_STATUS_OK;
+    job->iostatus = IO_STATUS_OK;
 }
 
 static void block_job_iostatus_reset(BlockJob *job)
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index fe433c8d35..fd7ba1a285 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -50,7 +50,7 @@ typedef struct BlockJob {
      * Status that is published by the query-block-jobs QMP API.
      * Protected by job mutex.
      */
-    BlockDeviceIoStatus iostatus;
+    IoStatus iostatus;
 
     /**
      * Speed that was set with @block_job_set_speed.
diff --git a/include/sysemu/block-backend-global-state.h b/include/sysemu/block-backend-global-state.h
index 49c12b0fa9..a45643438f 100644
--- a/include/sysemu/block-backend-global-state.h
+++ b/include/sysemu/block-backend-global-state.h
@@ -66,7 +66,7 @@ int GRAPH_UNLOCKED blk_set_perm(BlockBackend *blk, uint64_t perm,
 void blk_get_perm(BlockBackend *blk, uint64_t *perm, uint64_t *shared_perm);
 
 void blk_iostatus_enable(BlockBackend *blk);
-BlockDeviceIoStatus blk_iostatus(const BlockBackend *blk);
+IoStatus blk_iostatus(const BlockBackend *blk);
 void blk_iostatus_disable(BlockBackend *blk);
 void blk_iostatus_reset(BlockBackend *blk);
 int blk_attach_dev(BlockBackend *blk, DeviceState *dev);
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 93f96e747e..1f00d4f031 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -571,7 +571,7 @@
             'write_threshold': 'int', '*dirty-bitmaps': ['BlockDirtyInfo'] } }
 
 ##
-# @BlockDeviceIoStatus:
+# @IoStatus:
 #
 # An enumeration of block device I/O status.
 #
@@ -584,7 +584,7 @@
 #
 # Since: 1.0
 ##
-{ 'enum': 'BlockDeviceIoStatus', 'data': [ 'ok', 'failed', 'nospace' ] }
+{ 'enum': 'IoStatus', 'data': [ 'ok', 'failed', 'nospace' ] }
 
 ##
 # @BlockDirtyInfo:
@@ -705,7 +705,7 @@
 # @tray_open: True if the device's tray is open (only present if it
 #     has a tray)
 #
-# @io-status: @BlockDeviceIoStatus.  Only present if the device
+# @io-status: @IoStatus.  Only present if the device
 #     supports it and the VM is configured to stop on errors
 #     (supported device models: virtio-blk, IDE, SCSI except
 #     scsi-generic)
@@ -718,7 +718,7 @@
 { 'struct': 'BlockInfo',
   'data': {'device': 'str', '*qdev': 'str', 'type': 'str', 'removable': 'bool',
            'locked': 'bool', '*inserted': 'BlockDeviceInfo',
-           '*tray_open': 'bool', '*io-status': 'BlockDeviceIoStatus' } }
+           '*tray_open': 'bool', '*io-status': 'IoStatus' } }
 
 ##
 # @BlockMeasureInfo:
@@ -1424,7 +1424,7 @@
 { 'union': 'BlockJobInfo',
   'base': {'type': 'JobType', 'device': 'str', 'len': 'int',
            'offset': 'int', 'busy': 'bool', 'paused': 'bool', 'speed': 'int',
-           'io-status': 'BlockDeviceIoStatus', 'ready': 'bool',
+           'io-status': 'IoStatus', 'ready': 'bool',
            'status': 'JobStatus',
            'auto-finalize': 'bool', 'auto-dismiss': 'bool',
            '*error': 'str' },
-- 
2.34.1

[RFC 13/15] qapi: move IoStatus to common.json

[Vladimir Sementsov-Ogievskiy]

We will need to use the structure both in block-core.json and job.json.
So, move it to common.json, which could be then included to job.json.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 qapi/block-core.json | 16 ----------------
 qapi/common.json     | 16 ++++++++++++++++
 2 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 1f00d4f031..75c02e1586 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -570,22 +570,6 @@
             '*iops_size': 'int', '*group': 'str', 'cache': 'BlockdevCacheInfo',
             'write_threshold': 'int', '*dirty-bitmaps': ['BlockDirtyInfo'] } }
 
-##
-# @IoStatus:
-#
-# An enumeration of block device I/O status.
-#
-# @ok: The last I/O operation has succeeded
-#
-# @failed: The last I/O operation has failed
-#
-# @nospace: The last I/O operation has failed due to a no-space
-#     condition
-#
-# Since: 1.0
-##
-{ 'enum': 'IoStatus', 'data': [ 'ok', 'failed', 'nospace' ] }
-
 ##
 # @BlockDirtyInfo:
 #
diff --git a/qapi/common.json b/qapi/common.json
index f1bb841951..3becca1300 100644
--- a/qapi/common.json
+++ b/qapi/common.json
@@ -19,6 +19,22 @@
 { 'enum': 'IoOperationType',
   'data': [ 'read', 'write' ] }
 
+##
+# @IoStatus:
+#
+# An enumeration of block device I/O status.
+#
+# @ok: The last I/O operation has succeeded
+#
+# @failed: The last I/O operation has failed
+#
+# @nospace: The last I/O operation has failed due to a no-space
+#     condition
+#
+# Since: 1.0
+##
+{ 'enum': 'IoStatus', 'data': [ 'ok', 'failed', 'nospace' ] }
+
 ##
 # @OnOffAuto:
 #
-- 
2.34.1

[RFC 14/15] qapi: query-job: add block-job specific information

[Vladimir Sementsov-Ogievskiy]

Add io-status and speed, which make sense only for block-jobs. This
allows us to finally deprecate old query-block-jobs API in the next
commit.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 block/backup.c           |  6 ++++++
 block/commit.c           |  6 ++++++
 block/mirror.c           |  8 ++++++++
 block/stream.c           |  6 ++++++
 blockjob.c               | 10 ++++++++++
 include/block/blockjob.h |  6 ++++++
 qapi/job.json            | 21 ++++++++++++++++++++-
 7 files changed, 62 insertions(+), 1 deletion(-)

diff --git a/block/backup.c b/block/backup.c
index bf086dc5f9..55bbe85bf6 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -343,6 +343,11 @@ static bool backup_change(Job *job, JobChangeOptions *opts, Error **errp)
     return block_job_change(bjob, &opts->u.backup, errp);
 }
 
+static void backup_query(Job *job, JobInfo *info)
+{
+    block_job_query(job, &info->u.backup);
+}
+
 static const BlockJobDriver backup_job_driver = {
     .job_driver = {
         .instance_size          = sizeof(BackupBlockJob),
@@ -356,6 +361,7 @@ static const BlockJobDriver backup_job_driver = {
         .pause                  = backup_pause,
         .cancel                 = backup_cancel,
         .change                 = backup_change,
+        .query                  = backup_query,
     },
     .set_speed = backup_set_speed,
 };
diff --git a/block/commit.c b/block/commit.c
index ccb6097679..9199a6adc8 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -211,6 +211,11 @@ static bool commit_change(Job *job, JobChangeOptions *opts, Error **errp)
     return block_job_change(bjob, &opts->u.commit, errp);
 }
 
+static void commit_query(Job *job, JobInfo *info)
+{
+    block_job_query(job, &info->u.commit);
+}
+
 static const BlockJobDriver commit_job_driver = {
     .job_driver = {
         .instance_size = sizeof(CommitBlockJob),
@@ -222,6 +227,7 @@ static const BlockJobDriver commit_job_driver = {
         .abort         = commit_abort,
         .clean         = commit_clean,
         .change        = commit_change,
+        .query         = commit_query,
     },
 };
 
diff --git a/block/mirror.c b/block/mirror.c
index 8e672f3309..e8092d56be 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1310,6 +1310,8 @@ static void mirror_query(Job *job, JobInfo *info)
     info->u.mirror = (JobInfoMirror) {
         .actively_synced = qatomic_read(&s->actively_synced),
     };
+
+    block_job_query(job, qapi_JobInfoMirror_base(&info->u.mirror));
 }
 
 static const BlockJobDriver mirror_job_driver = {
@@ -1338,6 +1340,11 @@ static bool commit_active_change(Job *job, JobChangeOptions *opts, Error **errp)
     return block_job_change(bjob, &opts->u.commit, errp);
 }
 
+static void commit_active_query(Job *job, JobInfo *info)
+{
+    block_job_query(job, &info->u.commit);
+}
+
 static const BlockJobDriver commit_active_job_driver = {
     .job_driver = {
         .instance_size          = sizeof(MirrorBlockJob),
@@ -1351,6 +1358,7 @@ static const BlockJobDriver commit_active_job_driver = {
         .complete               = mirror_complete,
         .cancel                 = commit_active_cancel,
         .change                 = commit_active_change,
+        .query                  = commit_active_query,
     },
     .drained_poll           = mirror_drained_poll,
 };
diff --git a/block/stream.c b/block/stream.c
index 34f4588537..e5e4d0bc77 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -246,6 +246,11 @@ static bool stream_change(Job *job, JobChangeOptions *opts, Error **errp)
     return block_job_change(bjob, &opts->u.stream, errp);
 }
 
+static void stream_query(Job *job, JobInfo *info)
+{
+    block_job_query(job, &info->u.stream);
+}
+
 static const BlockJobDriver stream_job_driver = {
     .job_driver = {
         .instance_size = sizeof(StreamBlockJob),
@@ -256,6 +261,7 @@ static const BlockJobDriver stream_job_driver = {
         .clean         = stream_clean,
         .user_resume   = block_job_user_resume,
         .change        = stream_change,
+        .query         = stream_query,
     },
 };
 
diff --git a/blockjob.c b/blockjob.c
index de1dd03b2d..7dd1ed3ff2 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -306,6 +306,16 @@ bool block_job_set_speed_locked(BlockJob *job, int64_t speed, Error **errp)
     return true;
 }
 
+void block_job_query(Job *job, JobInfoBlockJob *info)
+{
+    BlockJob *bjob = container_of(job, BlockJob, job);
+
+    JOB_LOCK_GUARD();
+
+    info->speed = bjob->speed;
+    info->io_status = bjob->iostatus;
+}
+
 static bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)
 {
     JOB_LOCK_GUARD();
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index fd7ba1a285..bc33c2ba77 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -231,4 +231,10 @@ const BlockJobDriver *block_job_driver(BlockJob *job);
 bool block_job_change(BlockJob *job, JobChangeOptionsBlockJob *opts,
                       Error **errp);
 
+/**
+ * Common part of .query handler for block-jobs.
+ * Adds block-job specific information to @info.
+ */
+void block_job_query(Job *job, JobInfoBlockJob *info);
+
 #endif
diff --git a/qapi/job.json b/qapi/job.json
index 036fec1b57..7bd9f8112c 100644
--- a/qapi/job.json
+++ b/qapi/job.json
@@ -4,6 +4,7 @@
 ##
 # = Background jobs
 ##
+{ 'include': 'common.json' }
 
 ##
 # @JobType:
@@ -251,6 +252,20 @@
 ##
 { 'command': 'job-finalize', 'data': { 'id': 'str' } }
 
+##
+# @JobInfoBlockJob:
+#
+# Information specific to block jobs like mirror and backup.
+#
+# @io-status: the io status of the job
+#
+# @speed: the rate limit, bytes per second
+#
+# Since: 9.1
+##
+{ 'struct': 'JobInfoBlockJob',
+  'data': { 'io-status': 'IoStatus', 'speed': 'uint64' } }
+
 ##
 # @JobInfoMirror:
 #
@@ -263,6 +278,7 @@
 # Since: 9.1
 ##
 { 'struct': 'JobInfoMirror',
+  'base': 'JobInfoBlockJob',
   'data': { 'actively-synced': 'bool' } }
 
 ##
@@ -300,7 +316,10 @@
             'current-progress': 'int', 'total-progress': 'int',
             '*error': 'str' },
   'discriminator': 'type',
-  'data': { 'mirror': 'JobInfoMirror' } }
+  'data': { 'mirror': 'JobInfoMirror',
+            'backup': 'JobInfoBlockJob',
+            'stream': 'JobInfoBlockJob',
+            'commit': 'JobInfoBlockJob' } }
 
 ##
 # @query-jobs:
-- 
2.34.1

[RFC 15/15] qapi/block-core: derpecate block-job- APIs

[Vladimir Sementsov-Ogievskiy]

For change, pause, resume, complete, dismiss and finalize actions
corresponding job- and block-job commands are almost equal. The
difference is in find_block_job_locked() vs find_job_locked()
functions. What's different?

1. find_block_job_locked() do check, is found job a block-job. This OK
   when moving to more generic API, no needs to document this change.

2. find_block_job_locked() reports DeviceNotActive on failure, when
   find_job_locked() reports GenericError. So, lets document this
   difference in deprecated.txt. Still, for dismiss and finalize errors
   are not documented at all, so be silent in deprecated.txt as well.

For cancel, we have a new solution about soft-cancelling mirror:
job-complete(no-block-replace=true)

For set-speed, the action is supported by job-change.

For query, query-jobs is not equal to query-block-jobs, but now it has
enough information (see documentation changes for details).

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 docs/about/deprecated.rst | 73 +++++++++++++++++++++++++++++++++++++--
 qapi/block-core.json      | 59 ++++++++++++++++++++++++++++++-
 2 files changed, 129 insertions(+), 3 deletions(-)

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 5ff98ef95f..7db3ba983b 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -128,6 +128,75 @@ options are removed in favor of using explicit ``blockdev-create`` and
 ``blockdev-add`` calls. See :doc:`/interop/live-block-operations` for
 details.
 
+``block-job-pause`` (since 9.1)
+'''''''''''''''''''''''''''''''
+
+Use ``job-pause`` instead. The only difference is that ``job-pause``
+always reports GenericError on failure when ``block-job-pause`` reports
+DeviceNotActive when block-job is not found.
+
+``block-job-resume`` (since 9.1)
+''''''''''''''''''''''''''''''''
+
+Use ``job-resume`` instead. The only difference is that ``job-resume``
+always reports GenericError on failure when ``block-job-resume`` reports
+DeviceNotActive when block-job is not found.
+
+``block-job-complete`` (since 9.1)
+''''''''''''''''''''''''''''''''''
+
+Use ``job-complete`` instead. The only difference is that ``job-complete``
+always reports GenericError on failure when ``block-job-complete`` reports
+DeviceNotActive when block-job is not found.
+
+``block-job-dismiss`` (since 9.1)
+'''''''''''''''''''''''''''''''''
+
+Use ``job-dismiss`` instead.
+
+``block-job-finalize`` (since 9.1)
+''''''''''''''''''''''''''''''''''
+
+Use ``job-finalize`` instead.
+
+``block-job-set-speed`` (since 9.1)
+'''''''''''''''''''''''''''''''''''
+
+Use ``job-change`` instead.
+
+``block-job-change`` (since 9.1)
+''''''''''''''''''''''''''''''''
+
+Use ``job-change`` instead.
+
+``block-job-cancel`` (since 9.1)
+''''''''''''''''''''''''''''''''
+
+Use ``job-cancel`` instead which correspond to
+``block-job-cancel`` (``force`` = true ).  For special case of
+soft-cancelling mirror in ready state, use ``job-complete``
+(``no-block-replace`` = true ).
+
+``query-block-jobs`` (since 9.1)
+
+Use ``query-jobs`` instead. Per-field recommendations:
+
+``query-block-jobs`` ``device`` field: use ``query-jobs`` ``id`` field.
+
+``query-block-jobs`` ``len`` and ``offset`` fields: use ``query-jobs``
+``current-progress`` and ``total-progress`` fields.
+
+``query-block-jobs`` ``paused``, ``ready``:
+use ``qeury-jobs`` ``status``.
+
+``auto-finalize``, ``auto-dismiss``: these are parameters set by user
+on job creation and never changed. So, no reason to query them. No
+support in ``query-jobs``.
+
+``busy``: it actually means nothing for user, it's a mistake to rely on
+it. No support in ``query-jobs``.
+
+
 Incorrectly typed ``device_add`` arguments (since 6.2)
 ''''''''''''''''''''''''''''''''''''''''''''''''''''''
 
@@ -143,8 +212,8 @@ all arguments passed to ``device_add`` are consistent with the documented
 property types.
 
 
-``block-job-change`` and ``job-change``  argument ``type`` (since 9.1)
-''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
+``job-change`` argument ``type`` (since 9.1)
+''''''''''''''''''''''''''''''''''''''''''''
 
 QEMU can get job type from the job itself (by @id), @type field is redundant.
 
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 75c02e1586..793155f174 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1422,9 +1422,15 @@
 #
 # Returns: a list of @BlockJobInfo for each active block job
 #
+# Features:
+#
+# @deprecated: This command is deprecated.  Use @query-jobs
+#     instead.
+#
 # Since: 1.1
 ##
 { 'command': 'query-block-jobs', 'returns': ['BlockJobInfo'],
+  'features': ['deprecated'],
   'allow-preconfig': true }
 
 ##
@@ -2875,6 +2881,11 @@
 # @speed: the maximum speed, in bytes per second, or 0 for unlimited.
 #     Defaults to 0.
 #
+# Features:
+#
+# @deprecated: This command is deprecated.  Use @job-change
+#     instead.
+#
 # Errors:
 #     - If no background operation is active on this device,
 #       DeviceNotActive
@@ -2883,6 +2894,7 @@
 ##
 { 'command': 'block-job-set-speed',
   'data': { 'device': 'str', 'speed': 'int' },
+ 'features': ['deprecated'],
   'allow-preconfig': true }
 
 ##
@@ -2919,13 +2931,22 @@
 #     paused) instead of waiting for the destination to complete its
 #     final synchronization (since 1.3)
 #
+# Features:
+#
+# @deprecated: This command is deprecated.  Use @job-cancel
+#     instead which correspond to block-job-cancel(force=true).
+#     For special case of soft-cancelling mirror in ready state,
+#     use job-complete(no-block-replace=true) instead.
+#
 # Errors:
 #     - If no background operation is active on this device,
 #       DeviceNotActive
 #
 # Since: 1.1
 ##
-{ 'command': 'block-job-cancel', 'data': { 'device': 'str', '*force': 'bool' },
+{ 'command': 'block-job-cancel',
+  'features': ['deprecated'],
+  'data': { 'device': 'str', '*force': 'bool' },
   'allow-preconfig': true }
 
 ##
@@ -2945,6 +2966,11 @@
 #     the name of the parameter), but since QEMU 2.7 it can have other
 #     values.
 #
+# Features:
+#
+# @deprecated: This command is deprecated.  Use @job-pause
+#     instead.
+#
 # Errors:
 #     - If no background operation is active on this device,
 #       DeviceNotActive
@@ -2952,6 +2978,7 @@
 # Since: 1.3
 ##
 { 'command': 'block-job-pause', 'data': { 'device': 'str' },
+  'features': ['deprecated'],
   'allow-preconfig': true }
 
 ##
@@ -2969,6 +2996,11 @@
 #     the name of the parameter), but since QEMU 2.7 it can have other
 #     values.
 #
+# Features:
+#
+# @deprecated: This command is deprecated.  Use @job-resume
+#     instead.
+#
 # Errors:
 #     - If no background operation is active on this device,
 #       DeviceNotActive
@@ -2976,6 +3008,7 @@
 # Since: 1.3
 ##
 { 'command': 'block-job-resume', 'data': { 'device': 'str' },
+  'features': ['deprecated'],
   'allow-preconfig': true }
 
 ##
@@ -3000,6 +3033,11 @@
 #     the name of the parameter), but since QEMU 2.7 it can have other
 #     values.
 #
+# Features:
+#
+# @deprecated: This command is deprecated.  Use @job-complete
+#     instead.
+#
 # Errors:
 #     - If no background operation is active on this device,
 #       DeviceNotActive
@@ -3007,6 +3045,7 @@
 # Since: 1.3
 ##
 { 'command': 'block-job-complete', 'data': { 'device': 'str' },
+  'features': ['deprecated'],
   'allow-preconfig': true }
 
 ##
@@ -3024,9 +3063,15 @@
 #
 # @id: The job identifier.
 #
+# Features:
+#
+# @deprecated: This command is deprecated.  Use @job-dismiss
+#     instead.
+#
 # Since: 2.12
 ##
 { 'command': 'block-job-dismiss', 'data': { 'id': 'str' },
+  'features': ['deprecated'],
   'allow-preconfig': true }
 
 ##
@@ -3041,9 +3086,15 @@
 #
 # @id: The job identifier.
 #
+# Features:
+#
+# @deprecated: This command is deprecated.  Use @job-finalize
+#     instead.
+#
 # Since: 2.12
 ##
 { 'command': 'block-job-finalize', 'data': { 'id': 'str' },
+  'features': ['deprecated'],
   'allow-preconfig': true }
 
 ##
@@ -3103,9 +3154,15 @@
 #
 # Change the block job's options.
 #
+# Features:
+#
+# @deprecated: This command is deprecated.  Use @job-change
+#     instead.
+#
 # Since: 8.2
 ##
 { 'command': 'block-job-change',
+  'features': ['deprecated'],
   'data': 'JobChangeOptions', 'boxed': true }
 
 ##
-- 
2.34.1

Re: [RFC 01/15] scripts/qapi: support type-based unions

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:

> Look at block-job-change command: we have to specify both 'id' to chose
> the job to operate on and 'type' for QAPI union be parsed. But for user
> this looks redundant: when we specify 'id', QEMU should be able to get
> corresponding job's type.
>
> This commit brings such a possibility: just specify some Enum type as
> 'discriminator', and define a function somewhere with prototype
>
> bool YourUnionType_mapper(YourUnionType *, EnumType *out, Error **errp)
>
> Further commits will use this functionality to upgrade block-job-change
> interface and introduce some new interfaces.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  scripts/qapi/introspect.py |  5 +++-
>  scripts/qapi/schema.py     | 50 +++++++++++++++++++++++---------------
>  scripts/qapi/types.py      |  3 ++-
>  scripts/qapi/visit.py      | 43 ++++++++++++++++++++++++++------
>  4 files changed, 73 insertions(+), 28 deletions(-)

I believe you need to update docs/devel/qapi-code-gen.rst.

Current text:

    Union types
    -----------

    Syntax::

        UNION = { 'union': STRING,
                  'base': ( MEMBERS | STRING ),
                  'discriminator': STRING,
                  'data': BRANCHES,
                  '*if': COND,
                  '*features': FEATURES }
        BRANCHES = { BRANCH, ... }
        BRANCH = STRING : TYPE-REF
               | STRING : { 'type': TYPE-REF, '*if': COND }

    Member 'union' names the union type.

    The 'base' member defines the common members.  If it is a MEMBERS_
    object, it defines common members just like a struct type's 'data'
    member defines struct type members.  If it is a STRING, it names a
    struct type whose members are the common members.

    Member 'discriminator' must name a non-optional enum-typed member of
    the base struct.  That member's value selects a branch by its name.
    If no such branch exists, an empty branch is assumed.

If I understand your commit message correctly, this paragraph is no
longer true.

    Each BRANCH of the 'data' object defines a branch of the union.  A
    union must have at least one branch.

    The BRANCH's STRING name is the branch name.  It must be a value of
    the discriminator enum type.

    The BRANCH's value defines the branch's properties, in particular its
    type.  The type must a struct type.  The form TYPE-REF_ is shorthand
    for :code:`{ 'type': TYPE-REF }`.

    In the Client JSON Protocol, a union is represented by an object with
    the common members (from the base type) and the selected branch's
    members.  The two sets of member names must be disjoint.

    Example::

     { 'enum': 'BlockdevDriver', 'data': [ 'file', 'qcow2' ] }
     { 'union': 'BlockdevOptions',
       'base': { 'driver': 'BlockdevDriver', '*read-only': 'bool' },
       'discriminator': 'driver',
       'data': { 'file': 'BlockdevOptionsFile',
                 'qcow2': 'BlockdevOptionsQcow2' } }

    Resulting in these JSON objects::

     { "driver": "file", "read-only": true,
       "filename": "/some/place/my-image" }
     { "driver": "qcow2", "read-only": false,
       "backing": "/some/place/my-image", "lazy-refcounts": true }

    The order of branches need not match the order of the enum values.
    The branches need not cover all possible enum values.  In the
    resulting generated C data types, a union is represented as a struct
    with the base members in QAPI schema order, and then a union of
    structures for each branch of the struct.

    The optional 'if' member specifies a conditional.  See `Configuring
    the schema`_ below for more on this.

    The optional 'features' member specifies features.  See Features_
    below for more on this.

Re: [RFC 04/15] qapi: block-job-change: make copy-mode parameter optional

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:

> We are going to add more parameters to change. We want to make possible
> to change only one or any subset of available options. So all the
> options should be optional.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  block/mirror.c       | 5 +++++
>  qapi/block-core.json | 2 +-
>  2 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/block/mirror.c b/block/mirror.c
> index a177502e4f..2d0cd22c06 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -1265,6 +1265,11 @@ static void mirror_change(BlockJob *job, JobChangeOptions *opts,
>  
>      GLOBAL_STATE_CODE();
>  
> +    if (!change_opts->has_copy_mode) {
> +        /* Nothing to do */

I doubt the comment is useful.

> +        return;
> +    }
> +
>      if (qatomic_read(&s->copy_mode) == change_opts->copy_mode) {
>          return;
>      }

       if (change_opts->copy_mode != MIRROR_COPY_MODE_WRITE_BLOCKING) {
           error_setg(errp, "Change to copy mode '%s' is not implemented",
                      MirrorCopyMode_str(change_opts->copy_mode));
           return;
       }

       current = qatomic_cmpxchg(&s->copy_mode, MIRROR_COPY_MODE_BACKGROUND,
                                 change_opts->copy_mode);
       if (current != MIRROR_COPY_MODE_BACKGROUND) {
           error_setg(errp, "Expected current copy mode '%s', got '%s'",
                      MirrorCopyMode_str(MIRROR_COPY_MODE_BACKGROUND),
                      MirrorCopyMode_str(current));
       }

Now I'm curious: what could be changing @copy_mode behind our backs
here?

> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 67dd0ef038..6041e7bd8f 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -3071,7 +3071,7 @@
   ##
   # @BlockJobChangeOptionsMirror:
   #
   # @copy-mode: Switch to this copy mode.  Currently, only the switch
   #     from 'background' to 'write-blocking' is implemented.
   #
>  # Since: 8.2
>  ##
>  { 'struct': 'JobChangeOptionsMirror',
> -  'data': { 'copy-mode' : 'MirrorCopyMode' } }
> +  'data': { '*copy-mode' : 'MirrorCopyMode' } }
>  
>  ##
>  # @JobChangeOptions:

A member becoming optional is backward compatible.  Okay.

We may want to document "(optional since 9.1)".  We haven't done so in
the past.

The doc comment needs to spell out how absent members are handled.

Re: [RFC 01/15] scripts/qapi: support type-based unions

[Markus Armbruster]

Subject: all unions are type-based.  Perhaps "support implicit union
tags on the wire"?

Do you need this schema language feature for folding block jobs into the
jobs abstraction, or is it just for making the wire protocol nicer in
places?

Re: [RFC 01/15] scripts/qapi: support type-based unions

[Vladimir Sementsov-Ogievskiy]

On 28.03.24 12:40, Markus Armbruster wrote:
> Subject: all unions are type-based.  Perhaps "support implicit union
> tags on the wire"?

Yes, sounds good.

> 
> Do you need this schema language feature for folding block jobs into the
> jobs abstraction, or is it just for making the wire protocol nicer in
> places?
> 

It's not necessary, we can proceed with job-* API, specifying both type and id.

But I think, as we are not in a hurry, better to make new job-* API more effective from the beginning.

-- 
Best regards,
Vladimir

Re: [RFC 01/15] scripts/qapi: support type-based unions

[Vladimir Sementsov-Ogievskiy]

On 28.03.24 12:15, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
> 
>> Look at block-job-change command: we have to specify both 'id' to chose
>> the job to operate on and 'type' for QAPI union be parsed. But for user
>> this looks redundant: when we specify 'id', QEMU should be able to get
>> corresponding job's type.
>>
>> This commit brings such a possibility: just specify some Enum type as
>> 'discriminator', and define a function somewhere with prototype
>>
>> bool YourUnionType_mapper(YourUnionType *, EnumType *out, Error **errp)
>>
>> Further commits will use this functionality to upgrade block-job-change
>> interface and introduce some new interfaces.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> ---
>>   scripts/qapi/introspect.py |  5 +++-
>>   scripts/qapi/schema.py     | 50 +++++++++++++++++++++++---------------
>>   scripts/qapi/types.py      |  3 ++-
>>   scripts/qapi/visit.py      | 43 ++++++++++++++++++++++++++------
>>   4 files changed, 73 insertions(+), 28 deletions(-)
> 
> I believe you need to update docs/devel/qapi-code-gen.rst.
> 
> Current text:
> 
>      Union types
>      -----------
> 
>      Syntax::
> 
>          UNION = { 'union': STRING,
>                    'base': ( MEMBERS | STRING ),
>                    'discriminator': STRING,
>                    'data': BRANCHES,
>                    '*if': COND,
>                    '*features': FEATURES }
>          BRANCHES = { BRANCH, ... }
>          BRANCH = STRING : TYPE-REF
>                 | STRING : { 'type': TYPE-REF, '*if': COND }
> 
>      Member 'union' names the union type.
> 
>      The 'base' member defines the common members.  If it is a MEMBERS_
>      object, it defines common members just like a struct type's 'data'
>      member defines struct type members.  If it is a STRING, it names a
>      struct type whose members are the common members.
> 
>      Member 'discriminator' must name a non-optional enum-typed member of
>      the base struct.  That member's value selects a branch by its name.
>      If no such branch exists, an empty branch is assumed.
> 
> If I understand your commit message correctly, this paragraph is no
> longer true.


Right. Like this:

Member 'discriminator' must name either a non-optional enum-typed member, or an enum type name. (and more description follow, about user defined function and so on).


Do you think that mixing member name and type name here is OK? Or should I instead add another field 'discriminator-type', so that exactly one of 'discriminator' and 'discriminator-type' should be in union definition?


> 
>      Each BRANCH of the 'data' object defines a branch of the union.  A
>      union must have at least one branch.
> 
>      The BRANCH's STRING name is the branch name.  It must be a value of
>      the discriminator enum type.
> 
>      The BRANCH's value defines the branch's properties, in particular its
>      type.  The type must a struct type.  The form TYPE-REF_ is shorthand
>      for :code:`{ 'type': TYPE-REF }`.
> 
>      In the Client JSON Protocol, a union is represented by an object with
>      the common members (from the base type) and the selected branch's
>      members.  The two sets of member names must be disjoint.
> 
>      Example::
> 
>       { 'enum': 'BlockdevDriver', 'data': [ 'file', 'qcow2' ] }
>       { 'union': 'BlockdevOptions',
>         'base': { 'driver': 'BlockdevDriver', '*read-only': 'bool' },
>         'discriminator': 'driver',
>         'data': { 'file': 'BlockdevOptionsFile',
>                   'qcow2': 'BlockdevOptionsQcow2' } }
> 
>      Resulting in these JSON objects::
> 
>       { "driver": "file", "read-only": true,
>         "filename": "/some/place/my-image" }
>       { "driver": "qcow2", "read-only": false,
>         "backing": "/some/place/my-image", "lazy-refcounts": true }
> 
>      The order of branches need not match the order of the enum values.
>      The branches need not cover all possible enum values.  In the
>      resulting generated C data types, a union is represented as a struct
>      with the base members in QAPI schema order, and then a union of
>      structures for each branch of the struct.
> 
>      The optional 'if' member specifies a conditional.  See `Configuring
>      the schema`_ below for more on this.
> 
>      The optional 'features' member specifies features.  See Features_
>      below for more on this.
> 

-- 
Best regards,
Vladimir

Re: [RFC 04/15] qapi: block-job-change: make copy-mode parameter optional

[Vladimir Sementsov-Ogievskiy]

On 28.03.24 12:24, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
> 
>> We are going to add more parameters to change. We want to make possible
>> to change only one or any subset of available options. So all the
>> options should be optional.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> ---
>>   block/mirror.c       | 5 +++++
>>   qapi/block-core.json | 2 +-
>>   2 files changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/block/mirror.c b/block/mirror.c
>> index a177502e4f..2d0cd22c06 100644
>> --- a/block/mirror.c
>> +++ b/block/mirror.c
>> @@ -1265,6 +1265,11 @@ static void mirror_change(BlockJob *job, JobChangeOptions *opts,
>>   
>>       GLOBAL_STATE_CODE();
>>   
>> +    if (!change_opts->has_copy_mode) {
>> +        /* Nothing to do */
> 
> I doubt the comment is useful.
> 
>> +        return;
>> +    }
>> +
>>       if (qatomic_read(&s->copy_mode) == change_opts->copy_mode) {
>>           return;
>>       }
> 
>         if (change_opts->copy_mode != MIRROR_COPY_MODE_WRITE_BLOCKING) {
>             error_setg(errp, "Change to copy mode '%s' is not implemented",
>                        MirrorCopyMode_str(change_opts->copy_mode));
>             return;
>         }
> 
>         current = qatomic_cmpxchg(&s->copy_mode, MIRROR_COPY_MODE_BACKGROUND,
>                                   change_opts->copy_mode);
>         if (current != MIRROR_COPY_MODE_BACKGROUND) {
>             error_setg(errp, "Expected current copy mode '%s', got '%s'",
>                        MirrorCopyMode_str(MIRROR_COPY_MODE_BACKGROUND),
>                        MirrorCopyMode_str(current));
>         }
> 
> Now I'm curious: what could be changing @copy_mode behind our backs
> here?
> 

For now - nothing. But it may be read from another thread, so it's declared as atomic access.

So, we can check it with qatomic_read() instead, check the value first, and write then with qatomic_set().

But, I think it would be extra optimization (if it is). The operation is not often, and cmpxchg looks like a correct way to conditionally change atomic variable (even being a bit too safe)	.

>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 67dd0ef038..6041e7bd8f 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -3071,7 +3071,7 @@
>     ##
>     # @BlockJobChangeOptionsMirror:
>     #
>     # @copy-mode: Switch to this copy mode.  Currently, only the switch
>     #     from 'background' to 'write-blocking' is implemented.
>     #
>>   # Since: 8.2
>>   ##
>>   { 'struct': 'JobChangeOptionsMirror',
>> -  'data': { 'copy-mode' : 'MirrorCopyMode' } }
>> +  'data': { '*copy-mode' : 'MirrorCopyMode' } }
>>   
>>   ##
>>   # @JobChangeOptions:
> 
> A member becoming optional is backward compatible.  Okay.
> 
> We may want to document "(optional since 9.1)".  We haven't done so in
> the past.

Will do, I think it's useful.

> 
> The doc comment needs to spell out how absent members are handled.
> 

Will add.

-- 
Best regards,
Vladimir