| Message ID | 20220825143109.176582-8-alexander.ivanov@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | parallels: Refactor the code of images checks and fix a bug | expand |
On 25.08.2022 16:31, Alexander Ivanov wrote: > We will add more and more checks so we need a better code structure > in parallels_co_check. Let each check performs in a separate loop > in a separate helper. > > Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com> > --- > block/parallels.c | 59 ++++++++++++++++++++++++++++++++++------------- > 1 file changed, 43 insertions(+), 16 deletions(-) > > diff --git a/block/parallels.c b/block/parallels.c > index eea318f809..f50cd232aa 100644 > --- a/block/parallels.c > +++ b/block/parallels.c > @@ -438,13 +438,50 @@ static void parallels_check_unclean(BlockDriverState *bs, > } > } > > +static int parallels_check_outside_image(BlockDriverState *bs, > + BdrvCheckResult *res, > + BdrvCheckMode fix) > +{ > + BDRVParallelsState *s = bs->opaque; > + uint32_t i; > + int64_t off, high_off, size; > + > + size = bdrv_getlength(bs->file->bs); > + if (size < 0) { > + res->check_errors++; > + return size; > + } > + > + high_off = 0; > + for (i = 0; i < s->bat_size; i++) { > + off = bat2sect(s, i) << BDRV_SECTOR_BITS; > + if (off > size) { we need one more patch here. Correct check would be if (off >= size) { bla-bla() } > + fprintf(stderr, "%s cluster %u is outside image\n", > + fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i); > + res->corruptions++; > + if (fix & BDRV_FIX_ERRORS) { > + parallels_set_bat_entry(s, i, 0); > + res->corruptions_fixed++; > + } > + continue; > + } > + if (high_off < off) { > + high_off = off; > + } > + } > + > + s->data_end = (high_off + s->cluster_size) >> BDRV_SECTOR_BITS; > + > + return 0; > +} > + > static int coroutine_fn parallels_co_check(BlockDriverState *bs, > BdrvCheckResult *res, > BdrvCheckMode fix) > { > BDRVParallelsState *s = bs->opaque; > int64_t size, prev_off, high_off; > - int ret = 0; > + int ret; > uint32_t i; > > size = bdrv_getlength(bs->file->bs); > @@ -457,6 +494,11 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs, > > parallels_check_unclean(bs, res, fix); > > + ret = parallels_check_outside_image(bs, res, fix); > + if (ret < 0) { > + goto out; > + } > + > res->bfi.total_clusters = s->bat_size; > res->bfi.compressed_clusters = 0; /* compression is not supported */ > > @@ -469,19 +511,6 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs, > continue; > } > > - /* cluster outside the image */ > - if (off > size) { > - fprintf(stderr, "%s cluster %u is outside image\n", > - fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i); > - res->corruptions++; > - if (fix & BDRV_FIX_ERRORS) { > - parallels_set_bat_entry(s, i, 0); > - res->corruptions_fixed++; > - } > - prev_off = 0; > - continue; > - } > - > res->bfi.allocated_clusters++; > if (off > high_off) { > high_off = off; > @@ -519,8 +548,6 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs, > } > } > > - s->data_end = res->image_end_offset >> BDRV_SECTOR_BITS; > - > out: > qemu_co_mutex_unlock(&s->lock); > Reviewed-by: Denis V. Lunev <den@openvz.org>
On 26.08.2022 15:08, Denis V. Lunev wrote: > On 25.08.2022 16:31, Alexander Ivanov wrote: >> We will add more and more checks so we need a better code structure >> in parallels_co_check. Let each check performs in a separate loop >> in a separate helper. >> >> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com> >> --- >> block/parallels.c | 59 ++++++++++++++++++++++++++++++++++------------- >> 1 file changed, 43 insertions(+), 16 deletions(-) >> >> diff --git a/block/parallels.c b/block/parallels.c >> index eea318f809..f50cd232aa 100644 >> --- a/block/parallels.c >> +++ b/block/parallels.c >> @@ -438,13 +438,50 @@ static void >> parallels_check_unclean(BlockDriverState *bs, >> } >> } >> +static int parallels_check_outside_image(BlockDriverState *bs, >> + BdrvCheckResult *res, >> + BdrvCheckMode fix) >> +{ >> + BDRVParallelsState *s = bs->opaque; >> + uint32_t i; >> + int64_t off, high_off, size; >> + >> + size = bdrv_getlength(bs->file->bs); >> + if (size < 0) { >> + res->check_errors++; >> + return size; >> + } >> + >> + high_off = 0; >> + for (i = 0; i < s->bat_size; i++) { >> + off = bat2sect(s, i) << BDRV_SECTOR_BITS; >> + if (off > size) { > > we need one more patch here. Correct check would be > > if (off >= size) { > bla-bla() > } I have such a patch in the next patchset. > >> + fprintf(stderr, "%s cluster %u is outside image\n", >> + fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i); >> + res->corruptions++; >> + if (fix & BDRV_FIX_ERRORS) { >> + parallels_set_bat_entry(s, i, 0); >> + res->corruptions_fixed++; >> + } >> + continue; >> + } >> + if (high_off < off) { >> + high_off = off; >> + } >> + } >> + >> + s->data_end = (high_off + s->cluster_size) >> BDRV_SECTOR_BITS; >> + >> + return 0; >> +} >> + >> static int coroutine_fn parallels_co_check(BlockDriverState *bs, >> BdrvCheckResult *res, >> BdrvCheckMode fix) >> { >> BDRVParallelsState *s = bs->opaque; >> int64_t size, prev_off, high_off; >> - int ret = 0; >> + int ret; >> uint32_t i; >> size = bdrv_getlength(bs->file->bs); >> @@ -457,6 +494,11 @@ static int coroutine_fn >> parallels_co_check(BlockDriverState *bs, >> parallels_check_unclean(bs, res, fix); >> + ret = parallels_check_outside_image(bs, res, fix); >> + if (ret < 0) { >> + goto out; >> + } >> + >> res->bfi.total_clusters = s->bat_size; >> res->bfi.compressed_clusters = 0; /* compression is not >> supported */ >> @@ -469,19 +511,6 @@ static int coroutine_fn >> parallels_co_check(BlockDriverState *bs, >> continue; >> } >> - /* cluster outside the image */ >> - if (off > size) { >> - fprintf(stderr, "%s cluster %u is outside image\n", >> - fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i); >> - res->corruptions++; >> - if (fix & BDRV_FIX_ERRORS) { >> - parallels_set_bat_entry(s, i, 0); >> - res->corruptions_fixed++; >> - } >> - prev_off = 0; >> - continue; >> - } >> - >> res->bfi.allocated_clusters++; >> if (off > high_off) { >> high_off = off; >> @@ -519,8 +548,6 @@ static int coroutine_fn >> parallels_co_check(BlockDriverState *bs, >> } >> } >> - s->data_end = res->image_end_offset >> BDRV_SECTOR_BITS; >> - >> out: >> qemu_co_mutex_unlock(&s->lock); > Reviewed-by: Denis V. Lunev <den@openvz.org>
On 26.08.2022 15:23, Alexander Ivanov wrote: > > On 26.08.2022 15:08, Denis V. Lunev wrote: >> On 25.08.2022 16:31, Alexander Ivanov wrote: >>> We will add more and more checks so we need a better code structure >>> in parallels_co_check. Let each check performs in a separate loop >>> in a separate helper. >>> >>> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com> >>> --- >>> block/parallels.c | 59 >>> ++++++++++++++++++++++++++++++++++------------- >>> 1 file changed, 43 insertions(+), 16 deletions(-) >>> >>> diff --git a/block/parallels.c b/block/parallels.c >>> index eea318f809..f50cd232aa 100644 >>> --- a/block/parallels.c >>> +++ b/block/parallels.c >>> @@ -438,13 +438,50 @@ static void >>> parallels_check_unclean(BlockDriverState *bs, >>> } >>> } >>> +static int parallels_check_outside_image(BlockDriverState *bs, >>> + BdrvCheckResult *res, >>> + BdrvCheckMode fix) >>> +{ >>> + BDRVParallelsState *s = bs->opaque; >>> + uint32_t i; >>> + int64_t off, high_off, size; >>> + >>> + size = bdrv_getlength(bs->file->bs); >>> + if (size < 0) { >>> + res->check_errors++; >>> + return size; >>> + } >>> + >>> + high_off = 0; >>> + for (i = 0; i < s->bat_size; i++) { >>> + off = bat2sect(s, i) << BDRV_SECTOR_BITS; >>> + if (off > size) { >> >> we need one more patch here. Correct check would be >> >> if (off >= size) { >> bla-bla() >> } > I have such a patch in the next patchset. If you have it in hands, can you pls send it here as 11/10 patch. That would be most convenient to me. Den
diff --git a/block/parallels.c b/block/parallels.c index eea318f809..f50cd232aa 100644 --- a/block/parallels.c +++ b/block/parallels.c @@ -438,13 +438,50 @@ static void parallels_check_unclean(BlockDriverState *bs, } } +static int parallels_check_outside_image(BlockDriverState *bs, + BdrvCheckResult *res, + BdrvCheckMode fix) +{ + BDRVParallelsState *s = bs->opaque; + uint32_t i; + int64_t off, high_off, size; + + size = bdrv_getlength(bs->file->bs); + if (size < 0) { + res->check_errors++; + return size; + } + + high_off = 0; + for (i = 0; i < s->bat_size; i++) { + off = bat2sect(s, i) << BDRV_SECTOR_BITS; + if (off > size) { + fprintf(stderr, "%s cluster %u is outside image\n", + fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i); + res->corruptions++; + if (fix & BDRV_FIX_ERRORS) { + parallels_set_bat_entry(s, i, 0); + res->corruptions_fixed++; + } + continue; + } + if (high_off < off) { + high_off = off; + } + } + + s->data_end = (high_off + s->cluster_size) >> BDRV_SECTOR_BITS; + + return 0; +} + static int coroutine_fn parallels_co_check(BlockDriverState *bs, BdrvCheckResult *res, BdrvCheckMode fix) { BDRVParallelsState *s = bs->opaque; int64_t size, prev_off, high_off; - int ret = 0; + int ret; uint32_t i; size = bdrv_getlength(bs->file->bs); @@ -457,6 +494,11 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs, parallels_check_unclean(bs, res, fix); + ret = parallels_check_outside_image(bs, res, fix); + if (ret < 0) { + goto out; + } + res->bfi.total_clusters = s->bat_size; res->bfi.compressed_clusters = 0; /* compression is not supported */ @@ -469,19 +511,6 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs, continue; } - /* cluster outside the image */ - if (off > size) { - fprintf(stderr, "%s cluster %u is outside image\n", - fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i); - res->corruptions++; - if (fix & BDRV_FIX_ERRORS) { - parallels_set_bat_entry(s, i, 0); - res->corruptions_fixed++; - } - prev_off = 0; - continue; - } - res->bfi.allocated_clusters++; if (off > high_off) { high_off = off; @@ -519,8 +548,6 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs, } } - s->data_end = res->image_end_offset >> BDRV_SECTOR_BITS; - out: qemu_co_mutex_unlock(&s->lock);
We will add more and more checks so we need a better code structure in parallels_co_check. Let each check performs in a separate loop in a separate helper. Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com> --- block/parallels.c | 59 ++++++++++++++++++++++++++++++++++------------- 1 file changed, 43 insertions(+), 16 deletions(-)