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

[Kevin Wolf]

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 <address@hidden>
> >---
> >  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