| Message ID | 20170629184320.7151-2-el13635@mail.ntua.gr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 06/29/2017 01:43 PM, Manos Pitsidianakis wrote: > The following functions fail if bs->drv does not implement them: > > bdrv_probe_blocksizes > bdrv_probe_geometry > bdrv_truncate > bdrv_has_zero_init > bdrv_get_info > bdrv_media_changed > bdrv_eject > bdrv_lock_medium > bdrv_co_ioctl > > Instead, the call should be passed to bs->file if it exists, to allow > filter drivers to support those methods without implementing them. > > Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr> > --- > block.c | 27 +++++++++++++++++++++++++-- > block/io.c | 5 +++++ > 2 files changed, 30 insertions(+), 2 deletions(-) Did you check whether any other existing drivers can be cleaned up to rely on the defaults? Or is it just the raw driver that you cleaned in 2/3? At any rate, this makes sense to me. However, > @@ -4205,6 +4222,8 @@ int bdrv_media_changed(BlockDriverState *bs) > > if (drv && drv->bdrv_media_changed) { > return drv->bdrv_media_changed(bs); > + } else if (drv && bs->file && bs->file->bs) { > + bdrv_media_changed(bs->file->bs); > } > return -ENOTSUP; This one returns -ENOTSUP unconditionally after recursing; looks like you omitted 'return'. If that's the only fix you make for v3, you can add: Reviewed-by: Eric Blake <eblake@redhat.com>
On 07/03/2017 10:56 AM, Eric Blake wrote: > On 06/29/2017 01:43 PM, Manos Pitsidianakis wrote: >> The following functions fail if bs->drv does not implement them: One other suggestion: most patches in the tree use 'topic: Capital', while yours used 'topic: lowercase'. It's probably worth posting a v3 that addresses all of the nits I've pointed out, including tweaking the commit messages.
On Mon, Jul 03, 2017 at 01:40:57PM -0500, Eric Blake wrote: > On 07/03/2017 10:56 AM, Eric Blake wrote: > > On 06/29/2017 01:43 PM, Manos Pitsidianakis wrote: > >> The following functions fail if bs->drv does not implement them: > > One other suggestion: most patches in the tree use 'topic: Capital', > while yours used 'topic: lowercase'. It's probably worth posting a v3 > that addresses all of the nits I've pointed out, including tweaking the > commit messages. Plenty of regular contributors use "topic: lowercase" including Michael Tsirkin, Mark Cave-Ayland, Peter Xu, and myself (just from looking at the first page of git log --oneline on qemu.git/master). If we want to enforce a consistent style in the future then it should be discussed separately.
On Thu, Jun 29, 2017 at 09:43:18PM +0300, Manos Pitsidianakis wrote: > diff --git a/block.c b/block.c > index 69439628..9e8d34ad 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 && bs->file && bs->file->bs) { > + return bdrv_probe_blocksizes(bs->file->bs, bsz); > } > > return -ENOTSUP; Currently only raw-format.c and file-posix.c implement bdrv_probe_blocksizes(). qcow2 will start reporting bs->file's blocksizes after this patch. This can lead to a change in blocksizes when live migrating from an old QEMU to a new QEMU. Guest operating systems and applications can be confused if the device suddenly changes beneath them. On the other hand, it's already possible to hit that today by migrating from a raw image to a qcow2 image. I think this change makes sense - we should propagate blocksizes through the graph - but it may introduce an incompatibility. Kevin: Do you think this is safe? > @@ -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 && bs->file->bs) { > + return bdrv_truncate(bs->file, offset, errp); > + } > error_setg(errp, "Image format driver does not support resize"); > return -ENOTSUP; > } This is not safe because existing image formats (e.g. vmdk, dmg) do not implement .bdrv_truncate(). If we begin truncating the underlying host file ("foo.vmdk") the disk image will be corrupted. It is only safe to forward .bdrv_truncate() in filter drivers. I suggest providing a default implementation instead: int bdrv_truncate_file(...); Then filters can do: .bdrv_truncate = bdrv_truncate_file, > diff --git a/block/io.c b/block/io.c > index c72d7015..a1aee01d 100644 > --- a/block/io.c > +++ b/block/io.c > @@ -2401,6 +2401,11 @@ int bdrv_co_ioctl(BlockDriverState *bs, int req, void *buf) > }; > BlockAIOCB *acb; > > + if (drv && !drv->bdrv_aio_ioctl && !drv->bdrv_co_ioctl && > + bs->file && bs->file->bs) { > + return bdrv_co_ioctl(bs->file->bs, req, buf); > + } > + > bdrv_inc_in_flight(bs); > if (!drv || (!drv->bdrv_aio_ioctl && !drv->bdrv_co_ioctl)) { > co.ret = -ENOTSUP; This operation cannot be allowed by default for the same reason as .bdrv_truncate(). It could change the host file in ways that the format driver can't handle. A separate function is needed here: bdrv_co_ioctl_file().
diff --git a/block.c b/block.c index 69439628..9e8d34ad 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 && bs->file && bs->file->bs) { + 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 && bs->file && bs->file->bs) { + 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 && bs->file->bs) { + 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->file->bs) { + 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 && bs->file->bs) { + return bdrv_get_info(bs->file->bs, bdi); + } return -ENOTSUP; + } memset(bdi, 0, sizeof(*bdi)); return drv->bdrv_get_info(bs, bdi); } @@ -4205,6 +4222,8 @@ int bdrv_media_changed(BlockDriverState *bs) if (drv && drv->bdrv_media_changed) { return drv->bdrv_media_changed(bs); + } else if (drv && bs->file && bs->file->bs) { + bdrv_media_changed(bs->file->bs); } return -ENOTSUP; } @@ -4218,6 +4237,8 @@ void bdrv_eject(BlockDriverState *bs, bool eject_flag) if (drv && drv->bdrv_eject) { drv->bdrv_eject(bs, eject_flag); + } else if (drv && bs->file && bs->file->bs) { + bdrv_eject(bs->file->bs, eject_flag); } } @@ -4233,6 +4254,8 @@ void bdrv_lock_medium(BlockDriverState *bs, bool locked) if (drv && drv->bdrv_lock_medium) { drv->bdrv_lock_medium(bs, locked); + } else if (drv && bs->file && bs->file->bs) { + bdrv_lock_medium(bs->file->bs, locked); } } diff --git a/block/io.c b/block/io.c index c72d7015..a1aee01d 100644 --- a/block/io.c +++ b/block/io.c @@ -2401,6 +2401,11 @@ int bdrv_co_ioctl(BlockDriverState *bs, int req, void *buf) }; BlockAIOCB *acb; + if (drv && !drv->bdrv_aio_ioctl && !drv->bdrv_co_ioctl && + bs->file && bs->file->bs) { + return bdrv_co_ioctl(bs->file->bs, req, buf); + } + bdrv_inc_in_flight(bs); if (!drv || (!drv->bdrv_aio_ioctl && !drv->bdrv_co_ioctl)) { co.ret = -ENOTSUP;
The following functions fail if bs->drv does not implement them: bdrv_probe_blocksizes bdrv_probe_geometry bdrv_truncate bdrv_has_zero_init bdrv_get_info bdrv_media_changed bdrv_eject bdrv_lock_medium bdrv_co_ioctl Instead, the call should be passed to bs->file if it exists, to allow filter drivers to support those methods without implementing them. Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr> --- block.c | 27 +++++++++++++++++++++++++-- block/io.c | 5 +++++ 2 files changed, 30 insertions(+), 2 deletions(-)