| Message ID | 20170711163748.17817-2-el13635@mail.ntua.gr |
|---|---|
| State | New |
| Headers | show |
On 07/11/2017 11:37 AM, Manos Pitsidianakis wrote: > The following functions fail if bs->drv is a filter and does not > implement them: > > bdrv_probe_blocksizes > bdrv_probe_geometry > bdrv_truncate > bdrv_has_zero_init > bdrv_get_info > > Instead, the call should be passed to bs->file if it exists, to allow > filter drivers to support those methods without implementing them. This > commit makes `drv->is_filter = true` imply that these callbacks will be > forwarded to bs->file by default, so disabling support for these > functions must be done explicitly. > > Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr> > --- > block.c | 21 +++++++++++++++++++-- > include/block/block_int.h | 6 +++++- > 2 files changed, 24 insertions(+), 3 deletions(-) > @@ -3778,6 +3786,9 @@ 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); > + } > > /* safe default */ > return 0; Someday, we should probably clean this function (and all callback implementations) to return bool rather than int. But not this patch. Certainly more conservative than the earlier versions. I'd still trust Kevin's review over mine, but looks okay to me. Reviewed-by: Eric Blake <eblake@redhat.com>
On Tue, Jul 11, 2017 at 07:37:45PM +0300, Manos Pitsidianakis wrote: > The following functions fail if bs->drv is a filter and does not > implement them: > > bdrv_probe_blocksizes > bdrv_probe_geometry > bdrv_truncate > bdrv_has_zero_init > bdrv_get_info > > Instead, the call should be passed to bs->file if it exists, to allow > filter drivers to support those methods without implementing them. This > commit makes `drv->is_filter = true` imply that these callbacks will be > forwarded to bs->file by default, so disabling support for these > functions must be done explicitly. > > Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr> > --- > block.c | 21 +++++++++++++++++++-- > include/block/block_int.h | 6 +++++- > 2 files changed, 24 insertions(+), 3 deletions(-) Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
On 07/11/2017 11:37 AM, Manos Pitsidianakis wrote: > The following functions fail if bs->drv is a filter and does not > implement them: > > bdrv_probe_blocksizes > bdrv_probe_geometry > bdrv_truncate > bdrv_has_zero_init > bdrv_get_info > > Instead, the call should be passed to bs->file if it exists, to allow > filter drivers to support those methods without implementing them. This > commit makes `drv->is_filter = true` imply that these callbacks will be > forwarded to bs->file by default, so disabling support for these > functions must be done explicitly. > > Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr> > --- > block.c | 21 +++++++++++++++++++-- > include/block/block_int.h | 6 +++++- > 2 files changed, 24 insertions(+), 3 deletions(-) > @@ -3406,11 +3410,15 @@ int bdrv_truncate(BdrvChild *child, int64_t offset, Error **errp) > > assert(child->perm & BLK_PERM_RESIZE); > > + /* if bs->drv == NULL, bs is closed, so there's nothing to do here */ > if (!drv) { > error_setg(errp, "No medium inserted"); > return -ENOMEDIUM; > } > if (!drv->bdrv_truncate) { > + if (bs->file && drv->is_filter) { > + return bdrv_truncate(bs->file, offset, errp); > + } This has a semantic (but not merge) conflict with Max's preallocation work, and it started to be non-trivial for me to adjust locally in my testing. It may be easiest if you send a v5 on top of 'https://github.com/XanClic/qemu.git block', since Max already has a pending pull request: https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg02841.html
diff --git a/block.c b/block.c index 69439628..7f7530b6 100644 --- a/block.c +++ b/block.c @@ -494,6 +494,8 @@ int bdrv_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz) 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); } return -ENOTSUP; @@ -511,6 +513,8 @@ int bdrv_probe_geometry(BlockDriverState *bs, HDGeometry *geo) 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); } return -ENOTSUP; @@ -3406,11 +3410,15 @@ int bdrv_truncate(BdrvChild *child, int64_t offset, Error **errp) assert(child->perm & BLK_PERM_RESIZE); + /* if bs->drv == NULL, bs is closed, so there's nothing to do here */ if (!drv) { error_setg(errp, "No medium inserted"); return -ENOMEDIUM; } if (!drv->bdrv_truncate) { + if (bs->file && drv->is_filter) { + return bdrv_truncate(bs->file, offset, errp); + } error_setg(errp, "Image format driver does not support resize"); return -ENOTSUP; } @@ -3778,6 +3786,9 @@ 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); + } /* safe default */ return 0; @@ -3832,10 +3843,16 @@ void bdrv_get_backing_filename(BlockDriverState *bs, int bdrv_get_info(BlockDriverState *bs, BlockDriverInfo *bdi) { BlockDriver *drv = bs->drv; - if (!drv) + /* if bs->drv == NULL, bs is closed, so there's nothing to do here */ + if (!drv) { return -ENOMEDIUM; - if (!drv->bdrv_get_info) + } + if (!drv->bdrv_get_info) { + if (bs->file && drv->is_filter) { + return bdrv_get_info(bs->file->bs, bdi); + } return -ENOTSUP; + } memset(bdi, 0, sizeof(*bdi)); return drv->bdrv_get_info(bs, bdi); } diff --git a/include/block/block_int.h b/include/block/block_int.h index 15fa6021..75e93f72 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -87,7 +87,11 @@ struct BlockDriver { const char *format_name; int instance_size; - /* set to true if the BlockDriver is a block filter */ + /* set to true if the BlockDriver is a block filter. Block filters pass + * certain callbacks that refer to data (see block.c) to their bs->file if + * the driver doesn't implement them. Drivers that do not wish to forward + * must implement them and return -ENOTSUP. + */ bool is_filter; /* for snapshots block filter like Quorum can implement the * following recursive callback.
The following functions fail if bs->drv is a filter and does not implement them: bdrv_probe_blocksizes bdrv_probe_geometry bdrv_truncate bdrv_has_zero_init bdrv_get_info Instead, the call should be passed to bs->file if it exists, to allow filter drivers to support those methods without implementing them. This commit makes `drv->is_filter = true` imply that these callbacks will be forwarded to bs->file by default, so disabling support for these functions must be done explicitly. Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr> --- block.c | 21 +++++++++++++++++++-- include/block/block_int.h | 6 +++++- 2 files changed, 24 insertions(+), 3 deletions(-)