Re: [Qemu-devel] [PATCH 13/17] blockdev: Remove IF_* check for read-only blockdev_init

[Max Reitz <mreitz@redhat.com>]

On 2013-09-20 13:54, Kevin Wolf wrote:
> IF_NONE allows read-only, which makes forbidding it in this place
> for other types pretty much pointless.
>
> Instead, make sure that all devices for which the check would have
> errored out check in their init function that they don't get a read-only
> BlockDriverState. This catches even cases where IF_NONE and -device is
> used.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   blockdev.c                 | 6 ------
>   hw/block/m25p80.c          | 5 +++++
>   hw/block/xen_disk.c        | 6 ++++++
>   hw/sd/milkymist-memcard.c  | 4 ++++
>   hw/sd/omap_mmc.c           | 8 ++++++++
>   hw/sd/pl181.c              | 4 ++++
>   hw/sd/pxa2xx_mmci.c        | 4 ++++
>   hw/sd/sd.c                 | 5 +++++
>   hw/sd/sdhci.c              | 4 ++++
>   hw/sd/ssi-sd.c             | 3 +++
>   tests/qemu-iotests/051.out | 5 ++++-
>   11 files changed, 47 insertions(+), 7 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index bbdae31..ed4ba65 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -529,12 +529,6 @@ static DriveInfo *blockdev_init(QDict *bs_opts,
>       if (media == MEDIA_CDROM) {
>           /* CDROM is fine for any interface, don't check.  */
>           ro = 1;
> -    } else if (ro == 1) {
> -        if (type != IF_SCSI && type != IF_VIRTIO && type != IF_FLOPPY &&
> -            type != IF_NONE && type != IF_PFLASH) {
> -            error_report("read-only not supported by this bus type");
> -            goto err;
> -        }
>       }
>   
>       bdrv_flags |= ro ? 0 : BDRV_O_RDWR;
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 8c3b7f0..02a1544 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -624,6 +624,11 @@ static int m25p80_init(SSISlave *ss)
>       if (dinfo && dinfo->bdrv) {
>           DB_PRINT_L(0, "Binding to IF_MTD drive\n");
>           s->bdrv = dinfo->bdrv;
> +        if (bdrv_is_read_only(s->bdrv)) {
> +            fprintf(stderr, "Can't use a read-only drive");
> +            return 1;
> +        }
> +
>           /* FIXME: Move to late init */
>           if (bdrv_read(s->bdrv, 0, s->storage, DIV_ROUND_UP(s->size,
>                                                       BDRV_SECTOR_SIZE))) {
> diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
> index f35fc59..828c9d9 100644
> --- a/hw/block/xen_disk.c
> +++ b/hw/block/xen_disk.c
> @@ -39,6 +39,7 @@
>   #include "hw/xen/xen_backend.h"
>   #include "xen_blkif.h"
>   #include "sysemu/blockdev.h"
> +#include "qemu/error-report.h"
>   
>   /* ------------------------------------------------------------- */
>   
> @@ -829,6 +830,11 @@ static int blk_connect(struct XenDevice *xendev)
>           /* setup via qemu cmdline -> already setup for us */
>           xen_be_printf(&blkdev->xendev, 2, "get configured bdrv (cmdline setup)\n");
>           blkdev->bs = blkdev->dinfo->bdrv;
> +        if (bdrv_is_read_only(blkdev->bs) && !readonly) {
> +            error_report("Unexpected read-only drive");
> +            blkdev->bs = NULL;
> +            return -1;
> +        }
Just out of curiosity, why do you use error_report here but 
fprintf(stderr) in most other places?

Max

>           /* blkdev->bs is not create by us, we get a reference
>            * so we can bdrv_unref() unconditionally */
>           bdrv_ref(blkdev->bs);
> diff --git a/hw/sd/milkymist-memcard.c b/hw/sd/milkymist-memcard.c
> index 42613b3..d1168c9 100644
> --- a/hw/sd/milkymist-memcard.c
> +++ b/hw/sd/milkymist-memcard.c
> @@ -255,6 +255,10 @@ static int milkymist_memcard_init(SysBusDevice *dev)
>   
>       dinfo = drive_get_next(IF_SD);
>       s->card = sd_init(dinfo ? dinfo->bdrv : NULL, false);
> +    if (s->card == NULL) {
> +        return -1;
> +    }
> +
>       s->enabled = dinfo ? bdrv_is_inserted(dinfo->bdrv) : 0;
>   
>       memory_region_init_io(&s->regs_region, OBJECT(s), &memcard_mmio_ops, s,
> diff --git a/hw/sd/omap_mmc.c b/hw/sd/omap_mmc.c
> index bf5d1fb..8c6ab2e 100644
> --- a/hw/sd/omap_mmc.c
> +++ b/hw/sd/omap_mmc.c
> @@ -593,6 +593,10 @@ struct omap_mmc_s *omap_mmc_init(hwaddr base,
>   
>       /* Instantiate the storage */
>       s->card = sd_init(bd, false);
> +    if (s->card == NULL) {
> +        fprintf(stderr, "Failed to initialise SD card\n");
> +        exit(1);
> +    }
>   
>       return s;
>   }
> @@ -618,6 +622,10 @@ struct omap_mmc_s *omap2_mmc_init(struct omap_target_agent_s *ta,
>   
>       /* Instantiate the storage */
>       s->card = sd_init(bd, false);
> +    if (s->card == NULL) {
> +        fprintf(stderr, "Failed to initialise SD card\n");
> +        exit(1);
> +    }
>   
>       s->cdet = qemu_allocate_irqs(omap_mmc_cover_cb, s, 1)[0];
>       sd_set_cb(s->card, NULL, s->cdet);
> diff --git a/hw/sd/pl181.c b/hw/sd/pl181.c
> index 03875bf..c35896d 100644
> --- a/hw/sd/pl181.c
> +++ b/hw/sd/pl181.c
> @@ -491,6 +491,10 @@ static int pl181_init(SysBusDevice *sbd)
>       qdev_init_gpio_out(dev, s->cardstatus, 2);
>       dinfo = drive_get_next(IF_SD);
>       s->card = sd_init(dinfo ? dinfo->bdrv : NULL, false);
> +    if (s->card == NULL) {
> +        return -1;
> +    }
> +
>       return 0;
>   }
>   
> diff --git a/hw/sd/pxa2xx_mmci.c b/hw/sd/pxa2xx_mmci.c
> index 90c955f..d720bc2 100644
> --- a/hw/sd/pxa2xx_mmci.c
> +++ b/hw/sd/pxa2xx_mmci.c
> @@ -539,6 +539,10 @@ PXA2xxMMCIState *pxa2xx_mmci_init(MemoryRegion *sysmem,
>   
>       /* Instantiate the actual storage */
>       s->card = sd_init(bd, false);
> +    if (s->card == NULL) {
> +        fprintf(stderr, "Failed to initialise SD card\n");
> +        exit(1);
> +    }
>   
>       register_savevm(NULL, "pxa2xx_mmci", 0, 0,
>                       pxa2xx_mmci_save, pxa2xx_mmci_load, s);
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 346d86f..7380f06 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -494,6 +494,11 @@ SDState *sd_init(BlockDriverState *bs, bool is_spi)
>   {
>       SDState *sd;
>   
> +    if (bdrv_is_read_only(bs)) {
> +        fprintf(stderr, "sd_init: Cannot use read-only drive\n");
> +        return NULL;
> +    }
> +
>       sd = (SDState *) g_malloc0(sizeof(SDState));
>       sd->buf = qemu_blockalign(bs, 512);
>       sd->spi = is_spi;
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 1483e19..87cd1de 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -1166,6 +1166,10 @@ static void sdhci_initfn(Object *obj)
>   
>       di = drive_get_next(IF_SD);
>       s->card = sd_init(di ? di->bdrv : NULL, false);
> +    if (s->card == NULL) {
> +        fprintf(stderr, "Failed to initialise SD card\n");
> +        exit(1);
> +    }
>       s->eject_cb = qemu_allocate_irqs(sdhci_insert_eject_cb, s, 1)[0];
>       s->ro_cb = qemu_allocate_irqs(sdhci_card_readonly_cb, s, 1)[0];
>       sd_set_cb(s->card, s->ro_cb, s->eject_cb);
> diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
> index d47e237..1bb56c4 100644
> --- a/hw/sd/ssi-sd.c
> +++ b/hw/sd/ssi-sd.c
> @@ -246,6 +246,9 @@ static int ssi_sd_init(SSISlave *dev)
>       s->mode = SSI_SD_CMD;
>       dinfo = drive_get_next(IF_SD);
>       s->sd = sd_init(dinfo ? dinfo->bdrv : NULL, true);
> +    if (s->sd == NULL) {
> +        return -1;
> +    }
>       register_savevm(&dev->qdev, "ssi_sd", -1, 1, ssi_sd_save, ssi_sd_load, s);
>       return 0;
>   }
> diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out
> index 88e8fa7..bee161e 100644
> --- a/tests/qemu-iotests/051.out
> +++ b/tests/qemu-iotests/051.out
> @@ -128,7 +128,10 @@ QEMU X.Y.Z monitor - type 'help' for more information
>   (qemu) q^[[K^[[Dqu^[[K^[[D^[[Dqui^[[K^[[D^[[D^[[Dquit^[[K
>   
>   Testing: -drive file=TEST_DIR/t.qcow2,if=ide,readonly=on
> -QEMU_PROG: -drive file=TEST_DIR/t.qcow2,if=ide,readonly=on: read-only not supported by this bus type
> +QEMU X.Y.Z monitor - type 'help' for more information
> +(qemu) QEMU_PROG: Can't use a read-only drive
> +QEMU_PROG: Device initialization failed.
> +QEMU_PROG: Initialization of device ide-hd failed
>   
>   Testing: -drive file=TEST_DIR/t.qcow2,if=virtio,readonly=on
>   QEMU X.Y.Z monitor - type 'help' for more information