Message ID | 20200625152215.941773-13-mreitz@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | block: Deal with filters | expand |
On 25.06.2020 18:21, Max Reitz wrote: > Places that use patterns like > > if (bs->drv->is_filter && bs->file) { > ... something about bs->file->bs ... > } > > should be > > BlockDriverState *filtered = bdrv_filter_bs(bs); > if (filtered) { > ... something about @filtered ... > } > > instead. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > block.c | 31 ++++++++++++++++++++----------- > block/io.c | 7 +++++-- > migration/block-dirty-bitmap.c | 8 +------- > 3 files changed, 26 insertions(+), 20 deletions(-) > ... > diff --git a/block/io.c b/block/io.c > index df8f2a98d4..385176b331 100644 > --- a/block/io.c > +++ b/block/io.c > @@ -3307,6 +3307,7 @@ int coroutine_fn bdrv_co_truncate(BdrvChild *child, int64_t offset, bool exact, > Error **errp) > { > BlockDriverState *bs = child->bs; > + BdrvChild *filtered; > BlockDriver *drv = bs->drv; > BdrvTrackedRequest req; > int64_t old_size, new_bytes; > @@ -3358,6 +3359,8 @@ int coroutine_fn bdrv_co_truncate(BdrvChild *child, int64_t offset, bool exact, > goto out; > } > > + filtered = bdrv_filter_child(bs); > + Isn't better to have this initialization right before the relevant if/else block? Andrey > /* > * If the image has a backing file that is large enough that it would > * provide data for the new area, we cannot leave it unallocated because > @@ -3390,8 +3393,8 @@ int coroutine_fn bdrv_co_truncate(BdrvChild *child, int64_t offset, bool exact, > goto out; > } > ret = drv->bdrv_co_truncate(bs, offset, exact, prealloc, flags, errp); > - } else if (bs->file && drv->is_filter) { > - ret = bdrv_co_truncate(bs->file, offset, exact, prealloc, flags, errp); > + } else if (filtered) { > + ret = bdrv_co_truncate(filtered, offset, exact, prealloc, flags, errp); > } else { > error_setg(errp, "Image format driver does not support resize"); > ret = -ENOTSUP; ... Reviewed-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
On 08.07.20 20:24, Andrey Shinkevich wrote: > On 25.06.2020 18:21, Max Reitz wrote: >> Places that use patterns like >> >> if (bs->drv->is_filter && bs->file) { >> ... something about bs->file->bs ... >> } >> >> should be >> >> BlockDriverState *filtered = bdrv_filter_bs(bs); >> if (filtered) { >> ... something about @filtered ... >> } >> >> instead. >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> block.c | 31 ++++++++++++++++++++----------- >> block/io.c | 7 +++++-- >> migration/block-dirty-bitmap.c | 8 +------- >> 3 files changed, 26 insertions(+), 20 deletions(-) >> > ... >> diff --git a/block/io.c b/block/io.c >> index df8f2a98d4..385176b331 100644 >> --- a/block/io.c >> +++ b/block/io.c >> @@ -3307,6 +3307,7 @@ int coroutine_fn bdrv_co_truncate(BdrvChild >> *child, int64_t offset, bool exact, >> Error **errp) >> { >> BlockDriverState *bs = child->bs; >> + BdrvChild *filtered; >> BlockDriver *drv = bs->drv; >> BdrvTrackedRequest req; >> int64_t old_size, new_bytes; >> @@ -3358,6 +3359,8 @@ int coroutine_fn bdrv_co_truncate(BdrvChild >> *child, int64_t offset, bool exact, >> goto out; >> } >> + filtered = bdrv_filter_child(bs); >> + > > Isn't better to have this initialization right before the relevant > if/else block? Hm, well, yes. In this case, though, maybe not. Patch 16 will add another BdrvChild to be initialized here (@backing), and we need to initialize that one here. So I felt it made sense to group them together. They got split up when I decided to put @filtered into this patch and @backing into its own. So now it may look a bit weird, but I feel like after patch 16 it makes sense. (I’m indifferent, basically.) Max
On 09.07.2020 11:59, Max Reitz wrote: > On 08.07.20 20:24, Andrey Shinkevich wrote: >> On 25.06.2020 18:21, Max Reitz wrote: >>> Places that use patterns like >>> >>> if (bs->drv->is_filter && bs->file) { >>> ... something about bs->file->bs ... >>> } >>> >>> should be >>> >>> BlockDriverState *filtered = bdrv_filter_bs(bs); >>> if (filtered) { >>> ... something about @filtered ... >>> } >>> >>> instead. >>> >>> Signed-off-by: Max Reitz <mreitz@redhat.com> >>> --- >>> block.c | 31 ++++++++++++++++++++----------- >>> block/io.c | 7 +++++-- >>> migration/block-dirty-bitmap.c | 8 +------- >>> 3 files changed, 26 insertions(+), 20 deletions(-) >>> >> ... >>> diff --git a/block/io.c b/block/io.c >>> index df8f2a98d4..385176b331 100644 >>> --- a/block/io.c >>> +++ b/block/io.c >>> @@ -3307,6 +3307,7 @@ int coroutine_fn bdrv_co_truncate(BdrvChild >>> *child, int64_t offset, bool exact, >>> Error **errp) >>> { >>> BlockDriverState *bs = child->bs; >>> + BdrvChild *filtered; >>> BlockDriver *drv = bs->drv; >>> BdrvTrackedRequest req; >>> int64_t old_size, new_bytes; >>> @@ -3358,6 +3359,8 @@ int coroutine_fn bdrv_co_truncate(BdrvChild >>> *child, int64_t offset, bool exact, >>> goto out; >>> } >>> + filtered = bdrv_filter_child(bs); >>> + >> Isn't better to have this initialization right before the relevant >> if/else block? > Hm, well, yes. In this case, though, maybe not. Patch 16 will add > another BdrvChild to be initialized here (@backing), and we need to > initialize that one here. So I felt it made sense to group them together. > > They got split up when I decided to put @filtered into this patch and > @backing into its own. So now it may look a bit weird, but I feel like > after patch 16 it makes sense. > > (I’m indifferent, basically.) > > Max Yes, it makes a sence. I am on the way to reviewing further and have not reached the 16th yet. It is a minor thing anyway )) Thank you for your response. Andrey
diff --git a/block.c b/block.c index 6449f3a11d..a44af9c3c1 100644 --- a/block.c +++ b/block.c @@ -710,11 +710,12 @@ int coroutine_fn bdrv_co_delete_file(BlockDriverState *bs, Error **errp) int bdrv_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz) { BlockDriver *drv = bs->drv; + BlockDriverState *filtered = bdrv_filter_bs(bs); if (drv && drv->bdrv_probe_blocksizes) { return drv->bdrv_probe_blocksizes(bs, bsz); - } else if (drv && drv->is_filter && bs->file) { - return bdrv_probe_blocksizes(bs->file->bs, bsz); + } else if (filtered) { + return bdrv_probe_blocksizes(filtered, bsz); } return -ENOTSUP; @@ -729,11 +730,12 @@ int bdrv_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz) int bdrv_probe_geometry(BlockDriverState *bs, HDGeometry *geo) { BlockDriver *drv = bs->drv; + BlockDriverState *filtered = bdrv_filter_bs(bs); if (drv && drv->bdrv_probe_geometry) { return drv->bdrv_probe_geometry(bs, geo); - } else if (drv && drv->is_filter && bs->file) { - return bdrv_probe_geometry(bs->file->bs, geo); + } else if (filtered) { + return bdrv_probe_geometry(filtered, geo); } return -ENOTSUP; @@ -5421,6 +5423,8 @@ int bdrv_has_zero_init_1(BlockDriverState *bs) int bdrv_has_zero_init(BlockDriverState *bs) { + BlockDriverState *filtered; + if (!bs->drv) { return 0; } @@ -5433,8 +5437,10 @@ int bdrv_has_zero_init(BlockDriverState *bs) if (bs->drv->bdrv_has_zero_init) { return bs->drv->bdrv_has_zero_init(bs); } - if (bs->file && bs->drv->is_filter) { - return bdrv_has_zero_init(bs->file->bs); + + filtered = bdrv_filter_bs(bs); + if (filtered) { + return bdrv_has_zero_init(filtered); } /* safe default */ @@ -5479,8 +5485,9 @@ int bdrv_get_info(BlockDriverState *bs, BlockDriverInfo *bdi) return -ENOMEDIUM; } if (!drv->bdrv_get_info) { - if (bs->file && drv->is_filter) { - return bdrv_get_info(bs->file->bs, bdi); + BlockDriverState *filtered = bdrv_filter_bs(bs); + if (filtered) { + return bdrv_get_info(filtered, bdi); } return -ENOTSUP; } @@ -6546,6 +6553,8 @@ int bdrv_amend_options(BlockDriverState *bs, QemuOpts *opts, bool bdrv_recurse_can_replace(BlockDriverState *bs, BlockDriverState *to_replace) { + BlockDriverState *filtered; + if (!bs || !bs->drv) { return false; } @@ -6560,9 +6569,9 @@ bool bdrv_recurse_can_replace(BlockDriverState *bs, } /* For filters without an own implementation, we can recurse on our own */ - if (bs->drv->is_filter) { - BdrvChild *child = bs->file ?: bs->backing; - return bdrv_recurse_can_replace(child->bs, to_replace); + filtered = bdrv_filter_bs(bs); + if (filtered) { + return bdrv_recurse_can_replace(filtered, to_replace); } /* Safe default */ diff --git a/block/io.c b/block/io.c index df8f2a98d4..385176b331 100644 --- a/block/io.c +++ b/block/io.c @@ -3307,6 +3307,7 @@ int coroutine_fn bdrv_co_truncate(BdrvChild *child, int64_t offset, bool exact, Error **errp) { BlockDriverState *bs = child->bs; + BdrvChild *filtered; BlockDriver *drv = bs->drv; BdrvTrackedRequest req; int64_t old_size, new_bytes; @@ -3358,6 +3359,8 @@ int coroutine_fn bdrv_co_truncate(BdrvChild *child, int64_t offset, bool exact, goto out; } + filtered = bdrv_filter_child(bs); + /* * If the image has a backing file that is large enough that it would * provide data for the new area, we cannot leave it unallocated because @@ -3390,8 +3393,8 @@ int coroutine_fn bdrv_co_truncate(BdrvChild *child, int64_t offset, bool exact, goto out; } ret = drv->bdrv_co_truncate(bs, offset, exact, prealloc, flags, errp); - } else if (bs->file && drv->is_filter) { - ret = bdrv_co_truncate(bs->file, offset, exact, prealloc, flags, errp); + } else if (filtered) { + ret = bdrv_co_truncate(filtered, offset, exact, prealloc, flags, errp); } else { error_setg(errp, "Image format driver does not support resize"); ret = -ENOTSUP; diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c index 47bc0f650c..dec656c074 100644 --- a/migration/block-dirty-bitmap.c +++ b/migration/block-dirty-bitmap.c @@ -356,13 +356,7 @@ static int init_dirty_bitmap_migration(void) while (bs && bs->drv && bs->drv->is_filter && !bdrv_has_named_bitmaps(bs)) { - if (bs->backing) { - bs = bs->backing->bs; - } else if (bs->file) { - bs = bs->file->bs; - } else { - bs = NULL; - } + bs = bdrv_filter_bs(bs); } if (bs && bs->drv && !bs->drv->is_filter) {
Places that use patterns like if (bs->drv->is_filter && bs->file) { ... something about bs->file->bs ... } should be BlockDriverState *filtered = bdrv_filter_bs(bs); if (filtered) { ... something about @filtered ... } instead. Signed-off-by: Max Reitz <mreitz@redhat.com> --- block.c | 31 ++++++++++++++++++++----------- block/io.c | 7 +++++-- migration/block-dirty-bitmap.c | 8 +------- 3 files changed, 26 insertions(+), 20 deletions(-)