| Message ID | 20170713100117.11961-2-el13635@mail.ntua.gr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 07/13/2017 05:01 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. > > Reviewed-by: Eric Blake <eblake@redhat.com> I would have dropped R-b on the grounds that you added comments to 'is_filter' (good! but non-trivial), to make sure it gets a grammar/wording review of the new content. Furthermore, your patch had a semantic change due to the rebase, and looking at that change... > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> > Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr> > --- > @@ -3420,11 +3424,15 @@ int bdrv_truncate(BdrvChild *child, int64_t offset, PreallocMode prealloc, > > 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, prealloc, offset, errp); ...ouch - you got it wrong. s/prealloc, offset/offset, prealloc/ > +++ 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; I'm okay with the new comment. With the swapped parameter order fixed (the maintainer can probably do that), Reviewed-by: Eric Blake <eblake@redhat.com>
On Thu, Jul 13, 2017 at 06:23:14AM -0500, Eric Blake wrote: >On 07/13/2017 05:01 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. >> >> Reviewed-by: Eric Blake <eblake@redhat.com> > >I would have dropped R-b on the grounds that you added comments to >'is_filter' (good! but non-trivial), to make sure it gets a >grammar/wording review of the new content. Furthermore, your patch had >a semantic change due to the rebase, and looking at that change... > >> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> >> Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr> >> --- > >> @@ -3420,11 +3424,15 @@ int bdrv_truncate(BdrvChild *child, int64_t offset, PreallocMode prealloc, >> >> 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, prealloc, offset, errp); > >...ouch - you got it wrong. s/prealloc, offset/offset, prealloc/ > >> +++ 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; > >I'm okay with the new comment. > >With the swapped parameter order fixed (the maintainer can probably do >that), >Reviewed-by: Eric Blake <eblake@redhat.com> Oh, this compiled correctly because PreallocMode is an enum. Sorry for the inconvenience, that was rushed. I will send a new version to be typical.
diff --git a/block.c b/block.c index d24ae85868..892f29d231 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; @@ -3420,11 +3424,15 @@ int bdrv_truncate(BdrvChild *child, int64_t offset, PreallocMode prealloc, 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, prealloc, offset, errp); + } error_setg(errp, "Image format driver does not support resize"); return -ENOTSUP; } @@ -3761,6 +3769,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; @@ -3815,10 +3826,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 669a2797fd..37388ccb18 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.