On Wed, 20 Jul 2011 18:24:19 +0200
Markus Armbruster <arm...@redhat.com> wrote:
> BlockDriverState member removable is a confused mess. It is true when
> an ide-cd, scsi-cd or floppy qdev is attached, or when the
> BlockDriverState was created with -drive if={floppy,sd} or -drive
> if={ide,scsi,xen,none},media=cdrom ("created removable"), except when
> an ide-hd, scsi-hd, scsi-generic or virtio-blk qdev is attached.
>
> Three users remain:
>
> 1. eject_device(), via bdrv_is_removable() uses it to determine
> whether a block device can eject media.
>
> 2. bdrv_info() is monitor command "info block". QMP documentation
> says "true if the device is removable, false otherwise". From the
> monitor user's point of view, the only sensible interpretation of
> "is removable" is "can eject media with monitor commands eject and
> change".
>
> A block device can eject media unless a device is attached that
> doesn't support it. Switch the two users over to new
> bdrv_dev_has_removable_media() that returns exactly that.
>
> 3. bdrv_getlength() uses to suppress its length cache when media can
> change (see commit 46a4e4e6). Media change is either monitor
> command change (updates the length cache), monitor command eject
> (doesn't update the length cache, easily fixable), or physical
> media change (invalidates length cache, not so easily fixable).
>
> I'm refraining from improving anything here, because this series is
> long enough already. Instead, I simply switch it over to
> bdrv_dev_has_removable_media() as well.
>
> This changes the behavior of the length cache and of monitor commands
> eject and change in two cases:
>
> a. drive not created removable, no device attached
>
> The commit makes the drive removable, and defeats the length cache.
>
> Example: -drive if=none
>
> b. drive created removable, but the attached drive is non-removable,
> and doesn't call bdrv_set_removable(..., 0) (most devices don't)
>
> The commit makes the drive non-removable, and enables the length
> cache.
>
> Example: -drive if=xen,media=cdrom -M xenpv
>
> The other non-removable devices that don't call
> bdrv_set_removable() can't currently use a drive created removable,
> either because they aren't qdevified, or because they lack a drive
> property. Won't stay that way.
>
> Signed-off-by: Markus Armbruster <arm...@redhat.com>
Looks good to me.
> ---
> block.c | 18 +++++++++++-------
> block.h | 3 ++-
> blockdev.c | 2 +-
> hw/scsi-disk.c | 5 +++++
> 4 files changed, 19 insertions(+), 9 deletions(-)
>
> diff --git a/block.c b/block.c
> index 9591c8a..b394f17 100644
> --- a/block.c
> +++ b/block.c
> @@ -750,6 +750,9 @@ void bdrv_set_dev_ops(BlockDriverState *bs, const
> BlockDevOps *ops,
> {
> bs->dev_ops = ops;
> bs->dev_opaque = opaque;
> + if (bdrv_dev_has_removable_media(bs) && bs == bs_snapshots) {
> + bs_snapshots = NULL;
> + }
> }
>
> static void bdrv_dev_change_media_cb(BlockDriverState *bs)
> @@ -759,6 +762,11 @@ static void bdrv_dev_change_media_cb(BlockDriverState
> *bs)
> }
> }
>
> +int bdrv_dev_has_removable_media(BlockDriverState *bs)
> +{
> + return !bs->dev || (bs->dev_ops && bs->dev_ops->change_media_cb);
> +}
> +
> bool bdrv_dev_is_medium_ejected(BlockDriverState *bs)
> {
> if (bs->dev_ops && bs->dev_ops->is_medium_ejected) {
> @@ -1198,7 +1206,7 @@ int64_t bdrv_getlength(BlockDriverState *bs)
> if (!drv)
> return -ENOMEDIUM;
>
> - if (bs->growable || bs->removable) {
> + if (bs->growable || bdrv_dev_has_removable_media(bs)) {
> if (drv->bdrv_getlength) {
> return drv->bdrv_getlength(bs);
> }
> @@ -1483,11 +1491,6 @@ void bdrv_set_removable(BlockDriverState *bs, int
> removable)
> }
> }
>
> -int bdrv_is_removable(BlockDriverState *bs)
> -{
> - return bs->removable;
> -}
> -
> int bdrv_is_read_only(BlockDriverState *bs)
> {
> return bs->read_only;
> @@ -1765,7 +1768,8 @@ void bdrv_info(Monitor *mon, QObject **ret_data)
>
> bs_obj = qobject_from_jsonf("{ 'device': %s, 'type': 'unknown', "
> "'removable': %i, 'locked': %i }",
> - bs->device_name, bs->removable,
> + bs->device_name,
> + bdrv_dev_has_removable_media(bs),
> bdrv_dev_is_medium_locked(bs));
> bs_dict = qobject_to_qdict(bs_obj);
>
> diff --git a/block.h b/block.h
> index b034d51..f6fa3d0 100644
> --- a/block.h
> +++ b/block.h
> @@ -33,6 +33,7 @@ typedef struct BlockDevOps {
> * Runs when virtual media changed (monitor commands eject, change)
> * Beware: doesn't run when a host device's physical media
> * changes. Sure would be useful if it did.
> + * Device models with removable media must implement this callback.
> */
> void (*change_media_cb)(void *opaque);
> /*
> @@ -102,6 +103,7 @@ void bdrv_detach_dev(BlockDriverState *bs, void *dev);
> void *bdrv_get_attached_dev(BlockDriverState *bs);
> void bdrv_set_dev_ops(BlockDriverState *bs, const BlockDevOps *ops,
> void *opaque);
> +int bdrv_dev_has_removable_media(BlockDriverState *bs);
> bool bdrv_dev_is_medium_ejected(BlockDriverState *bs);
> int bdrv_dev_is_medium_locked(BlockDriverState *bs);
> int bdrv_read(BlockDriverState *bs, int64_t sector_num,
> @@ -207,7 +209,6 @@ void bdrv_set_on_error(BlockDriverState *bs,
> BlockErrorAction on_read_error,
> BlockErrorAction on_write_error);
> BlockErrorAction bdrv_get_on_error(BlockDriverState *bs, int is_read);
> void bdrv_set_removable(BlockDriverState *bs, int removable);
> -int bdrv_is_removable(BlockDriverState *bs);
> int bdrv_is_read_only(BlockDriverState *bs);
> int bdrv_is_sg(BlockDriverState *bs);
> int bdrv_enable_write_cache(BlockDriverState *bs);
> diff --git a/blockdev.c b/blockdev.c
> index 4c58128..06a82d3 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -645,7 +645,7 @@ out:
>
> static int eject_device(Monitor *mon, BlockDriverState *bs, int force)
> {
> - if (!bdrv_is_removable(bs)) {
> + if (!bdrv_dev_has_removable_media(bs)) {
> qerror_report(QERR_DEVICE_NOT_REMOVABLE, bdrv_get_device_name(bs));
> return -1;
> }
> diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
> index ad63814..ae194c5 100644
> --- a/hw/scsi-disk.c
> +++ b/hw/scsi-disk.c
> @@ -1276,6 +1276,10 @@ static void scsi_destroy(SCSIDevice *dev)
> blockdev_mark_auto_del(s->qdev.conf.bs);
> }
>
> +static void scsi_cd_change_media_cb(void *opaque)
> +{
> +}
> +
> static bool scsi_cd_is_medium_ejected(void *opaque)
> {
> return ((SCSIDiskState *)opaque)->tray_open;
> @@ -1287,6 +1291,7 @@ static bool scsi_cd_is_medium_locked(void *opaque)
> }
>
> static const BlockDevOps scsi_cd_block_ops = {
> + .change_media_cb = scsi_cd_change_media_cb,
> .is_medium_ejected = scsi_cd_is_medium_ejected,
> .is_medium_locked = scsi_cd_is_medium_locked,
> };
