This is nicer to do in the unified QMP interface that we have now,
because it lets us use the right terminology back at the user.
Signed-off-by: John Snow <jsnow@redhat.com>
---
block/backup.c | 13 ++++---------
blockdev.c | 10 ++++++++++
2 files changed, 14 insertions(+), 9 deletions(-)
diff --git a/block/backup.c b/block/backup.c
index e2729cf6fa..a64b768e24 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -566,6 +566,10 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
assert(bs);
assert(target);
+ /* QMP interface protects us from these cases */
+ assert(sync_mode != MIRROR_SYNC_MODE_INCREMENTAL);
+ assert(sync_bitmap || sync_mode != MIRROR_SYNC_MODE_BITMAP);
+
if (bs == target) {
error_setg(errp, "Source and target cannot be the same");
return NULL;
@@ -597,16 +601,7 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
return NULL;
}
- /* QMP interface should have handled translating this to bitmap mode */
- assert(sync_mode != MIRROR_SYNC_MODE_INCREMENTAL);
-
if (sync_mode == MIRROR_SYNC_MODE_BITMAP) {
- if (!sync_bitmap) {
- error_setg(errp, "must provide a valid bitmap name for "
- "'%s' sync mode", MirrorSyncMode_str(sync_mode));
- return NULL;
- }
-
/* If we need to write to this bitmap, check that we can: */
if (bitmap_mode != BITMAP_SYNC_MODE_NEVER &&
bdrv_dirty_bitmap_check(sync_bitmap, BDRV_BITMAP_DEFAULT, errp)) {
diff --git a/blockdev.c b/blockdev.c
index 3e30bc2ca7..f0b7da53b0 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3464,6 +3464,16 @@ static BlockJob *do_backup_common(BackupCommon *backup,
return NULL;
}
+ if ((backup->sync == MIRROR_SYNC_MODE_BITMAP) ||
+ (backup->sync == MIRROR_SYNC_MODE_INCREMENTAL)) {
+ /* done before desugaring 'incremental' to print the right message */
+ if (!backup->has_bitmap) {
+ error_setg(errp, "must provide a valid bitmap name for "
+ "'%s' sync mode", MirrorSyncMode_str(backup->sync));
+ return NULL;
+ }
+ }
+
if (backup->sync == MIRROR_SYNC_MODE_INCREMENTAL) {
if (backup->has_bitmap_mode &&
backup->bitmap_mode != BITMAP_SYNC_MODE_ON_SUCCESS) {
--
2.21.0
On 10.07.19 03:05, John Snow wrote:
> This is nicer to do in the unified QMP interface that we have now,
> because it lets us use the right terminology back at the user.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> block/backup.c | 13 ++++---------
> blockdev.c | 10 ++++++++++
> 2 files changed, 14 insertions(+), 9 deletions(-)
>
> diff --git a/block/backup.c b/block/backup.c
> index e2729cf6fa..a64b768e24 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -566,6 +566,10 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
> assert(bs);
> assert(target);
>
> + /* QMP interface protects us from these cases */
> + assert(sync_mode != MIRROR_SYNC_MODE_INCREMENTAL);
> + assert(sync_bitmap || sync_mode != MIRROR_SYNC_MODE_BITMAP);
Implication would be a nice operator sometimes.
("assert(sync_mode == MIRROR_SYNC_MODE_BITMAP -> sync_bitmap)")
(Can you do that in C++? No, you can’t overload bool’s operators, right?)
Reviewed-by: Max Reitz <mreitz@redhat.com>
On 7/10/19 12:11 PM, Max Reitz wrote:
> On 10.07.19 03:05, John Snow wrote:
>> This is nicer to do in the unified QMP interface that we have now,
>> because it lets us use the right terminology back at the user.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> block/backup.c | 13 ++++---------
>> blockdev.c | 10 ++++++++++
>> 2 files changed, 14 insertions(+), 9 deletions(-)
>>
>> diff --git a/block/backup.c b/block/backup.c
>> index e2729cf6fa..a64b768e24 100644
>> --- a/block/backup.c
>> +++ b/block/backup.c
>> @@ -566,6 +566,10 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
>> assert(bs);
>> assert(target);
>>
>> + /* QMP interface protects us from these cases */
>> + assert(sync_mode != MIRROR_SYNC_MODE_INCREMENTAL);
>> + assert(sync_bitmap || sync_mode != MIRROR_SYNC_MODE_BITMAP);
>
> Implication would be a nice operator sometimes.
>
> ("assert(sync_mode == MIRROR_SYNC_MODE_BITMAP -> sync_bitmap)")
>
> (Can you do that in C++? No, you can’t overload bool’s operators, right?)
>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
>
Yes, I also find this assertion kind of hard to read personally, but it
feels somewhat clunky to write:
if (antecedent) {
assert(condition);
}
I suppose we can also phrase this as:
assert(sync_mode == MIRROR_SYNC_MODE_BITMAP ? sync_bitmap : true);
Which might honestly be pretty good. Mind if I change it to this?
--js
On 10.07.19 19:57, John Snow wrote:
>
>
> On 7/10/19 12:11 PM, Max Reitz wrote:
>> On 10.07.19 03:05, John Snow wrote:
>>> This is nicer to do in the unified QMP interface that we have now,
>>> because it lets us use the right terminology back at the user.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>> block/backup.c | 13 ++++---------
>>> blockdev.c | 10 ++++++++++
>>> 2 files changed, 14 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/block/backup.c b/block/backup.c
>>> index e2729cf6fa..a64b768e24 100644
>>> --- a/block/backup.c
>>> +++ b/block/backup.c
>>> @@ -566,6 +566,10 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
>>> assert(bs);
>>> assert(target);
>>>
>>> + /* QMP interface protects us from these cases */
>>> + assert(sync_mode != MIRROR_SYNC_MODE_INCREMENTAL);
>>> + assert(sync_bitmap || sync_mode != MIRROR_SYNC_MODE_BITMAP);
>>
>> Implication would be a nice operator sometimes.
>>
>> ("assert(sync_mode == MIRROR_SYNC_MODE_BITMAP -> sync_bitmap)")
>>
>> (Can you do that in C++? No, you can’t overload bool’s operators, right?)
>>
>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>>
>
> Yes, I also find this assertion kind of hard to read personally, but it
> feels somewhat clunky to write:
>
> if (antecedent) {
> assert(condition);
> }
>
> I suppose we can also phrase this as:
>
> assert(sync_mode == MIRROR_SYNC_MODE_BITMAP ? sync_bitmap : true);
>
> Which might honestly be pretty good. Mind if I change it to this?
Looks weird (mostly unfamiliar), but I do not.
Max
© 2016 - 2026 Red Hat, Inc.