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>
Acked-by: Markus Armbruster <armbru@redhat.com>
---
 block/mirror.c       | 4 ++++
 qapi/block-core.json | 3 ++-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/block/mirror.c b/block/mirror.c
index 2816bb1042..60e8d83e4f 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1272,6 +1272,10 @@ static void mirror_change(BlockJob *job, JobChangeOptions *opts,
 
     GLOBAL_STATE_CODE();
 
+    if (!change_opts->has_copy_mode) {
+        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 0156762024..f370593037 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3072,11 +3072,12 @@
 #
 # @copy-mode: Switch to this copy mode.  Currently, only the switch
 #     from 'background' to 'write-blocking' is implemented.
+#     If absent, copy mode remains the same.  (optional since 9.2)
 #
 # Since: 8.2
 ##
 { 'struct': 'JobChangeOptionsMirror',
-  'data': { 'copy-mode' : 'MirrorCopyMode' } }
+  'data': { '*copy-mode' : 'MirrorCopyMode' } }
 
 ##
 # @JobChangeOptions:
-- 
2.34.1

Am 02.10.2024 um 16:06 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 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>
> Acked-by: Markus Armbruster <armbru@redhat.com>

This is different from blockdev-reopen, which requires repeating all
options (and can therefore reuse the type from blockdev-add). One of the
reasons why we made it like that is that we had some options for which
"option absent" was a meaningful value in itself, so it would have
become ambiguous if an absent option in blockdev-reopen should mean
"leave the existing value unchanged" or "unset the option".

Are we confident that this will never happen with job options? In case
of doubt, I would go for consistency with blockdev-reopen.

Either way, it would be good to document the reasoning for whatever
option we choose in the commit message.

Kevin

On 22.10.24 15:35, Kevin Wolf wrote:
> Am 02.10.2024 um 16:06 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 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>
>> Acked-by: Markus Armbruster <armbru@redhat.com>
> 
> This is different from blockdev-reopen, which requires repeating all
> options (and can therefore reuse the type from blockdev-add). One of the
> reasons why we made it like that is that we had some options for which
> "option absent" was a meaningful value in itself, so it would have
> become ambiguous if an absent option in blockdev-reopen should mean
> "leave the existing value unchanged" or "unset the option".
> 

Right.. Thanks for noting this. I'll try to apply blockdev-reopen approach here.

> Are we confident that this will never happen with job options? In case
> of doubt, I would go for consistency with blockdev-reopen.
> 
> Either way, it would be good to document the reasoning for whatever
> option we choose in the commit message.
> 
> Kevin
> 

-- 
Best regards,
Vladimir