| Message ID | 1379678070-14346-14-git-send-email-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
Am 23.09.2013 um 10:00 hat Max Reitz geschrieben: > 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? Variatio delectat. ;-) I guess fprintf + exit was a common pattern elsewhere, so I used it, whereas error_report() is my default function. But now that I look at this file again, maybe it should really be xen_be_printf() here... Kevin
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; + } /* 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
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(-)