From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49352) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VO1CO-0004mP-8H for qemu-devel@nongnu.org; Mon, 23 Sep 2013 04:08:58 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VO1CI-0003XV-5l for qemu-devel@nongnu.org; Mon, 23 Sep 2013 04:08:52 -0400 Received: from mx1.redhat.com ([209.132.183.28]:8886) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VO1CH-0003XI-U2 for qemu-devel@nongnu.org; Mon, 23 Sep 2013 04:08:46 -0400 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r8N88hea015791 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Mon, 23 Sep 2013 04:08:44 -0400 Date: Mon, 23 Sep 2013 10:08:51 +0200 From: Kevin Wolf Message-ID: <20130923080850.GA2223@dhcp-200-207.str.redhat.com> References: <1379678070-14346-1-git-send-email-kwolf@redhat.com> <1379678070-14346-14-git-send-email-kwolf@redhat.com> <523FF528.2050007@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <523FF528.2050007@redhat.com> 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: Max Reitz Cc: qemu-devel@nongnu.org, stefanha@redhat.com, armbru@redhat.com 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 > >--- > > 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