Re: [RFC 04/15] qapi: block-job-change: make copy-mode parameter optional
On 28.03.24 12:24, Markus Armbruster wrote: Vladimir Sementsov-Ogievskiy 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 --- 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(>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(>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
Re: [RFC 04/15] qapi: block-job-change: make copy-mode parameter optional
Vladimir Sementsov-Ogievskiy 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 > --- > 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(>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(>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.
[RFC 04/15] qapi: block-job-change: make copy-mode parameter optional
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 --- 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(>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