From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47956) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VO14h-0003K5-I6 for qemu-devel@nongnu.org; Mon, 23 Sep 2013 04:01:01 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VO14b-0001KK-GG for qemu-devel@nongnu.org; Mon, 23 Sep 2013 04:00:55 -0400 Received: from mx1.redhat.com ([209.132.183.28]:35127) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VO14b-0001KC-7o for qemu-devel@nongnu.org; Mon, 23 Sep 2013 04:00:49 -0400 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r8N80lF2012448 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Mon, 23 Sep 2013 04:00:47 -0400 Message-ID: <523FF528.2050007@redhat.com> Date: Mon, 23 Sep 2013 10:00:40 +0200 From: Max Reitz MIME-Version: 1.0 References: <1379678070-14346-1-git-send-email-kwolf@redhat.com> <1379678070-14346-14-git-send-email-kwolf@redhat.com> In-Reply-To: <1379678070-14346-14-git-send-email-kwolf@redhat.com> Content-Type: text/plain; charset=ISO-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 13/17] blockdev: Remove IF_* check for read-only blockdev_init List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: qemu-devel@nongnu.org, stefanha@redhat.com, armbru@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 > --- > 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) qququiquit > > 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