| Message ID | 20190710010556.32365-5-jsnow@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | bitmaps: allow bitmaps to be used with full and top | expand |
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
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) {
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(-)