[Qemu-devel] [PATCH v2 00/13] Still more block related fixes and cleanups

[Qemu-devel] [PATCH v2 00/13] Still more block related fixes and cleanups

[Markus Armbruster]

I'm working on cleanly separating block device host and guest parts.
I'd like to route all this work through Kevin's block tree.  This is
still just preliminaries.

This patch series is based available at
git://repo.or.cz/qemu/armbru.git
tag block-fixes-2-v2: this series, based on
tag blockdev-base, which the current kevin/block

There will be at least one more round of cleanup & fixes before
blockdev_add proper.  I intend to start with a minimal QMP-only
version, then add features.

v2: rebased, better error messages (new 07/13 and 08/13)

Markus Armbruster (13):
  blockdev: Clean up how readonly persists across virtual media change
  block migration: Fix test for read-only drive
  raw-posix: Fix test for host CD-ROM
  fdc: Reject unimplemented error actions
  qdev: Don't hw_error() in qdev_init_nofail()
  scsi: Reject unimplemented error actions
  error: New qemu_opts_loc_restore()
  scsi: Error locations for -drive if=scsi device initialization
  ide: Improve error messages
  ide: Replace IDEState members is_cdrom, is_cf by drive_kind
  ide: Make ide_init_drive() return success
  ide: Reject readonly drives unless CD-ROM
  ide: Reject invalid CHS geometry

 block-migration.c   |    2 +-
 block/raw-posix.c   |   17 ++++--------
 blockdev.c          |    2 +-
 hw/fdc.c            |   22 +++++++++++----
 hw/ide/core.c       |   70 +++++++++++++++++++++++++++++++++-----------------
 hw/ide/internal.h   |    9 +++---
 hw/ide/macio.c      |    2 +-
 hw/ide/microdrive.c |    2 +-
 hw/ide/qdev.c       |   13 ++++++---
 hw/qdev.c           |    6 +++-
 hw/scsi-bus.c       |    4 +++
 hw/scsi-disk.c      |    5 +++
 hw/scsi-generic.c   |    9 ++++++
 qemu-option.c       |    5 +++
 qemu-option.h       |    1 +
 15 files changed, 113 insertions(+), 56 deletions(-)

[Qemu-devel] [PATCH v2 01/13] blockdev: Clean up how readonly persists across virtual media change

[Markus Armbruster]

Since commit cb4e5f8e, monitor command change makes the new media
readonly iff the type hint is BDRV_TYPE_CDROM, i.e. the drive was
created with media=cdrom.  The intention is to avoid changing a block
device's read-only-ness.  However, BDRV_TYPE_CDROM is only a hint.  It
is currently sufficent for read-only.  But it's not necessary, and it
may not remain sufficient.

Use bdrv_is_read_only() instead.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 blockdev.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index cecde2b..cca3eec 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -589,7 +589,7 @@ int do_change_block(Monitor *mon, const char *device,
     if (eject_device(mon, bs, 0) < 0) {
         return -1;
     }
-    bdrv_flags = bdrv_get_type_hint(bs) == BDRV_TYPE_CDROM ? 0 : BDRV_O_RDWR;
+    bdrv_flags = bdrv_is_read_only(bs) ? 0 : BDRV_O_RDWR;
     if (bdrv_open(bs, filename, bdrv_flags, drv) < 0) {
         qerror_report(QERR_OPEN_FILE_FAILED, filename);
         return -1;
-- 
1.6.6.1

[Qemu-devel] Re: [PATCH v2 01/13] blockdev: Clean up how readonly persists across virtual media change

[Christoph Hellwig]

Looks good,


Reviewed-by: Christoph Hellwig <hch@lst.de>

[Qemu-devel] [PATCH v2 02/13] block migration: Fix test for read-only drive

[Markus Armbruster]

init_blk_migration_it() skips drives with type hint BDRV_TYPE_CDROM.
The intention is to skip read-only drives.  However, BDRV_TYPE_CDROM
is only a hint.  It is currently sufficent for read-only.  But it's
not necessary, and it may not remain sufficient.

Use bdrv_is_read_only() instead.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block-migration.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/block-migration.c b/block-migration.c
index 7d04d6d..7337349 100644
--- a/block-migration.c
+++ b/block-migration.c
@@ -236,7 +236,7 @@ static void init_blk_migration_it(void *opaque, BlockDriverState *bs)
     BlkMigDevState *bmds;
     int64_t sectors;
 
-    if (bs->type == BDRV_TYPE_HD) {
+    if (!bdrv_is_read_only(bs)) {
         sectors = bdrv_getlength(bs) >> BDRV_SECTOR_BITS;
         if (sectors == 0) {
             return;
-- 
1.6.6.1

[Qemu-devel] Re: [PATCH v2 02/13] block migration: Fix test for read-only drive

[Christoph Hellwig]

Ok,


Reviewed-by: Christoph Hellwig <hch@lst.de>

[Qemu-devel] [PATCH v2 03/13] raw-posix: Fix test for host CD-ROM

[Markus Armbruster]

raw_pread_aligned() retries up to two times if the block device backs
a virtual CD-ROM (a drive with media=cdrom and if=ide, scsi, xen or
none).  This makes no sense.  Whether retrying reads can correct read
errors can only depend on what we're reading, not on how the result
gets used.  We need to check what whether we're reading from a
physical CD-ROM or floppy here.

I doubt retrying is useful even then.  Left for another day.

Impact:

* Virtual CD-ROM backed by host_cdrom behaves the same.

* Virtual CD-ROM backed by file or host_device no longer retries.

* A drive backed by host_cdrom now retries even if it's not a virtual
  CD-ROM.

* Any drive backed by host_floppy now retries.

While there, clean up gratuitous use of goto.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/raw-posix.c |   17 ++++++-----------
 1 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index 3f0701b..291699f 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -242,15 +242,14 @@ static int raw_pread_aligned(BlockDriverState *bs, int64_t offset,
 
     ret = pread(s->fd, buf, count, offset);
     if (ret == count)
-        goto label__raw_read__success;
+        return ret;
 
     /* Allow reads beyond the end (needed for pwrite) */
     if ((ret == 0) && bs->growable) {
         int64_t size = raw_getlength(bs);
         if (offset >= size) {
             memset(buf, 0, count);
-            ret = count;
-            goto label__raw_read__success;
+            return count;
         }
     }
 
@@ -260,13 +259,13 @@ static int raw_pread_aligned(BlockDriverState *bs, int64_t offset,
                       bs->total_sectors, ret, errno, strerror(errno));
 
     /* Try harder for CDrom. */
-    if (bs->type == BDRV_TYPE_CDROM) {
+    if (s->type != FTYPE_FILE) {
         ret = pread(s->fd, buf, count, offset);
         if (ret == count)
-            goto label__raw_read__success;
+            return ret;
         ret = pread(s->fd, buf, count, offset);
         if (ret == count)
-            goto label__raw_read__success;
+            return ret;
 
         DEBUG_BLOCK_PRINT("raw_pread(%d:%s, %" PRId64 ", %p, %d) [%" PRId64
                           "] retry read failed %d : %d = %s\n",
@@ -274,8 +273,6 @@ static int raw_pread_aligned(BlockDriverState *bs, int64_t offset,
                           bs->total_sectors, ret, errno, strerror(errno));
     }
 
-label__raw_read__success:
-
     return  (ret < 0) ? -errno : ret;
 }
 
@@ -298,15 +295,13 @@ static int raw_pwrite_aligned(BlockDriverState *bs, int64_t offset,
 
     ret = pwrite(s->fd, buf, count, offset);
     if (ret == count)
-        goto label__raw_write__success;
+        return ret;
 
     DEBUG_BLOCK_PRINT("raw_pwrite(%d:%s, %" PRId64 ", %p, %d) [%" PRId64
                       "] write failed %d : %d = %s\n",
                       s->fd, bs->filename, offset, buf, count,
                       bs->total_sectors, ret, errno, strerror(errno));
 
-label__raw_write__success:
-
     return  (ret < 0) ? -errno : ret;
 }
 
-- 
1.6.6.1

[Qemu-devel] Re: [PATCH v2 03/13] raw-posix: Fix test for host CD-ROM

[Christoph Hellwig]

On Tue, Jul 06, 2010 at 02:08:46PM +0200, Markus Armbruster wrote:
> * Any drive backed by host_floppy now retries.

I would really prefer not to change the behaviour for this case, it'll
just confuse people looking at the history when finally removing this
hack.

[Qemu-devel] Re: [PATCH v2 03/13] raw-posix: Fix test for host CD-ROM

[Markus Armbruster]

Christoph Hellwig <hch@lst.de> writes:

> On Tue, Jul 06, 2010 at 02:08:46PM +0200, Markus Armbruster wrote:
>> * Any drive backed by host_floppy now retries.
>
> I would really prefer not to change the behaviour for this case, it'll
> just confuse people looking at the history when finally removing this
> hack.

I'm fine either way.  Kevin?

[Qemu-devel] Re: [PATCH v2 03/13] raw-posix: Fix test for host CD-ROM

[Kevin Wolf]

Am 07.07.2010 11:34, schrieb Markus Armbruster:
> Christoph Hellwig <hch@lst.de> writes:
> 
>> On Tue, Jul 06, 2010 at 02:08:46PM +0200, Markus Armbruster wrote:
>>> * Any drive backed by host_floppy now retries.
>>
>> I would really prefer not to change the behaviour for this case, it'll
>> just confuse people looking at the history when finally removing this
>> hack.
> 
> I'm fine either way.  Kevin?

I don't really care about floppies, and I doubt anyone does. Retaining
old behaviour should never be wrong, so if you prefer, I'm okay with it.

Kevin

[Qemu-devel] [PATCH v2 04/13] fdc: Reject unimplemented error actions

[Markus Armbruster]

drive_init() doesn't permit them for if=floppy, but that's worthless:
we get them via if=none and -global.

This can make device initialization fail.  Since all callers of
fdctrl_init_isa() ignore its value, change it to die instead of
returning failure.  Without this, some callers would ignore the
failure, and others would crash.

Wart: unlike drive_init(), we don't reject the default action when
it's explicitly specified.  That's because we can't distinguish "no
rerror option" from "rerror=report", or "no werror" from
"rerror=enospc".  Left for another day.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/fdc.c |   22 ++++++++++++++++------
 1 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/hw/fdc.c b/hw/fdc.c
index 6c74878..2d50bd6 100644
--- a/hw/fdc.c
+++ b/hw/fdc.c
@@ -29,6 +29,7 @@
 
 #include "hw.h"
 #include "fdc.h"
+#include "qemu-error.h"
 #include "qemu-timer.h"
 #include "isa.h"
 #include "sysbus.h"
@@ -1844,7 +1845,7 @@ static void fdctrl_result_timer(void *opaque)
 }
 
 /* Init functions */
-static void fdctrl_connect_drives(FDCtrl *fdctrl)
+static int fdctrl_connect_drives(FDCtrl *fdctrl)
 {
     unsigned int i;
     FDrive *drive;
@@ -1852,12 +1853,24 @@ static void fdctrl_connect_drives(FDCtrl *fdctrl)
     for (i = 0; i < MAX_FD; i++) {
         drive = &fdctrl->drives[i];
 
+        if (drive->bs) {
+            if (bdrv_get_on_error(drive->bs, 0) != BLOCK_ERR_STOP_ENOSPC) {
+                error_report("fdc doesn't support drive option werror");
+                return -1;
+            }
+            if (bdrv_get_on_error(drive->bs, 1) != BLOCK_ERR_REPORT) {
+                error_report("fdc doesn't support drive option rerror");
+                return -1;
+            }
+        }
+
         fd_init(drive);
         fd_revalidate(drive);
         if (drive->bs) {
             bdrv_set_removable(drive->bs, 1);
         }
     }
+    return 0;
 }
 
 FDCtrl *fdctrl_init_isa(DriveInfo **fds)
@@ -1871,8 +1884,7 @@ FDCtrl *fdctrl_init_isa(DriveInfo **fds)
     if (fds[1]) {
         qdev_prop_set_drive_nofail(&dev->qdev, "driveB", fds[1]->bdrv);
     }
-    if (qdev_init(&dev->qdev) < 0)
-        return NULL;
+    qdev_init_nofail(&dev->qdev);
     return &(DO_UPCAST(FDCtrlISABus, busdev, dev)->state);
 }
 
@@ -1950,9 +1962,7 @@ static int fdctrl_init_common(FDCtrl *fdctrl)
 
     if (fdctrl->dma_chann != -1)
         DMA_register_channel(fdctrl->dma_chann, &fdctrl_transfer_handler, fdctrl);
-    fdctrl_connect_drives(fdctrl);
-
-    return 0;
+    return fdctrl_connect_drives(fdctrl);
 }
 
 static int isabus_fdc_init1(ISADevice *dev)
-- 
1.6.6.1

[Qemu-devel] Re: [PATCH v2 04/13] fdc: Reject unimplemented error actions

[Christoph Hellwig]

Looks good,


Reviewed-by: Christoph Hellwig <hch@lst.de>

[Qemu-devel] [PATCH v2 05/13] qdev: Don't hw_error() in qdev_init_nofail()

[Markus Armbruster]

Some of the failures are internal errors, and hw_error() is okay then.
But the common way to fail is bad user input, e.g. -global
isa-fdc.driveA=foo where drive foo has an unsupported rerror value.

exit(1) instead.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/qdev.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index 61f999c..00ceada 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -326,8 +326,10 @@ void qdev_init_nofail(DeviceState *dev)
 {
     DeviceInfo *info = dev->info;
 
-    if (qdev_init(dev) < 0)
-        hw_error("Initialization of device %s failed\n", info->name);
+    if (qdev_init(dev) < 0) {
+        error_report("Initialization of device %s failed\n", info->name);
+        exit(1);
+    }
 }
 
 /* Unlink device from bus and free the structure.  */
-- 
1.6.6.1

[Qemu-devel] Re: [PATCH v2 05/13] qdev: Don't hw_error() in qdev_init_nofail()

[Christoph Hellwig]

Looks good,


Reviewed-by: Christoph Hellwig <hch@lst.de>

[Qemu-devel] [PATCH v2 06/13] scsi: Reject unimplemented error actions

[Markus Armbruster]

drive_init() doesn't permit rerror for if=scsi, but that's worthless:
we get it via if=none and -device.

Moreover, scsi-generic doesn't support werror.  Since drive_init()
doesn't catch that, option werror was silently ignored even with
if=scsi.

Wart: unlike drive_init(), we don't reject the default action when
it's explicitly specified.  That's because we can't distinguish "no
rerror option" from "rerror=report", or "no werror" from
"rerror=enospc".  Left for another day.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/scsi-disk.c    |    5 +++++
 hw/scsi-generic.c |    9 +++++++++
 2 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 3e41011..c30709c 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -1059,6 +1059,11 @@ static int scsi_disk_initfn(SCSIDevice *dev)
     s->bs = s->qdev.conf.bs;
     is_cd = bdrv_get_type_hint(s->bs) == BDRV_TYPE_CDROM;
 
+    if (bdrv_get_on_error(s->bs, 1) != BLOCK_ERR_REPORT) {
+        error_report("Device doesn't support drive option rerror");
+        return -1;
+    }
+
     if (!s->serial) {
         /* try to fall back to value set with legacy -drive serial=... */
         dinfo = drive_get_by_blockdev(s->bs);
diff --git a/hw/scsi-generic.c b/hw/scsi-generic.c
index 3915e78..a8b4176 100644
--- a/hw/scsi-generic.c
+++ b/hw/scsi-generic.c
@@ -474,6 +474,15 @@ static int scsi_generic_initfn(SCSIDevice *dev)
         return -1;
     }
 
+    if (bdrv_get_on_error(s->bs, 0) != BLOCK_ERR_STOP_ENOSPC) {
+        error_report("Device doesn't support drive option werror");
+        return -1;
+    }
+    if (bdrv_get_on_error(s->bs, 1) != BLOCK_ERR_REPORT) {
+        error_report("Device doesn't support drive option rerror");
+        return -1;
+    }
+
     /* check we are using a driver managing SG_IO (version 3 and after */
     if (bdrv_ioctl(s->bs, SG_GET_VERSION_NUM, &sg_version) < 0 ||
         sg_version < 30000) {
-- 
1.6.6.1

[Qemu-devel] Re: [PATCH v2 06/13] scsi: Reject unimplemented error actions

[Christoph Hellwig]

On Tue, Jul 06, 2010 at 02:08:49PM +0200, Markus Armbruster wrote:
> drive_init() doesn't permit rerror for if=scsi, but that's worthless:
> we get it via if=none and -device.
> 
> Moreover, scsi-generic doesn't support werror.  Since drive_init()
> doesn't catch that, option werror was silently ignored even with
> if=scsi.
> 
> Wart: unlike drive_init(), we don't reject the default action when
> it's explicitly specified.  That's because we can't distinguish "no
> rerror option" from "rerror=report", or "no werror" from
> "rerror=enospc".  Left for another day.

I can't see a good reason that scsi doesn't support the rerror option,
and implementing is trivial.  So while this patch looks correct I'd
rather see rerror implemented for scsi than hacking around the lack of
it.

[Qemu-devel] Re: [PATCH v2 06/13] scsi: Reject unimplemented error actions

[Markus Armbruster]

Christoph Hellwig <hch@lst.de> writes:

> On Tue, Jul 06, 2010 at 02:08:49PM +0200, Markus Armbruster wrote:
>> drive_init() doesn't permit rerror for if=scsi, but that's worthless:
>> we get it via if=none and -device.
>> 
>> Moreover, scsi-generic doesn't support werror.  Since drive_init()
>> doesn't catch that, option werror was silently ignored even with
>> if=scsi.
>> 
>> Wart: unlike drive_init(), we don't reject the default action when
>> it's explicitly specified.  That's because we can't distinguish "no
>> rerror option" from "rerror=report", or "no werror" from
>> "rerror=enospc".  Left for another day.
>
> I can't see a good reason that scsi doesn't support the rerror option,
> and implementing is trivial.  So while this patch looks correct I'd
> rather see rerror implemented for scsi than hacking around the lack of
> it.

You got a point there.  Same for fdc.

However, this is the best *I* can do in time for .13.  Let's add the
missin error action support after the release, okay?

[Qemu-devel] [PATCH v2 07/13] error: New qemu_opts_loc_restore()

[Markus Armbruster]

Needed for decent error locations when complaining about options
outside of qemu_opts_foreach().  That one sets the location
already.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qemu-option.c |    5 +++++
 qemu-option.h |    1 +
 2 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/qemu-option.c b/qemu-option.c
index 30327d4..1f8f41a 100644
--- a/qemu-option.c
+++ b/qemu-option.c
@@ -728,6 +728,11 @@ void qemu_opts_reset(QemuOptsList *list)
     }
 }
 
+void qemu_opts_loc_restore(QemuOpts *opts)
+{
+    loc_restore(&opts->loc);
+}
+
 int qemu_opts_set(QemuOptsList *list, const char *id,
                   const char *name, const char *value)
 {
diff --git a/qemu-option.h b/qemu-option.h
index 9e2406c..b515813 100644
--- a/qemu-option.h
+++ b/qemu-option.h
@@ -116,6 +116,7 @@ int qemu_opt_foreach(QemuOpts *opts, qemu_opt_loopfunc func, void *opaque,
 QemuOpts *qemu_opts_find(QemuOptsList *list, const char *id);
 QemuOpts *qemu_opts_create(QemuOptsList *list, const char *id, int fail_if_exists);
 void qemu_opts_reset(QemuOptsList *list);
+void qemu_opts_loc_restore(QemuOpts *opts);
 int qemu_opts_set(QemuOptsList *list, const char *id,
                   const char *name, const char *value);
 const char *qemu_opts_id(QemuOpts *opts);
-- 
1.6.6.1

[Qemu-devel] Re: [PATCH v2 07/13] error: New qemu_opts_loc_restore()

[Christoph Hellwig]

On Tue, Jul 06, 2010 at 02:08:50PM +0200, Markus Armbruster wrote:
> Needed for decent error locations when complaining about options
> outside of qemu_opts_foreach().  That one sets the location
> already.

Ok.

[Qemu-devel] [PATCH v2 08/13] scsi: Error locations for -drive if=scsi device initialization

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/scsi-bus.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index b84b9b9..d69c74c 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -102,19 +102,23 @@ SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, BlockDriverState *bdrv, int
 
 int scsi_bus_legacy_handle_cmdline(SCSIBus *bus)
 {
+    Location loc;
     DriveInfo *dinfo;
     int res = 0, unit;
 
+    loc_push_none(&loc);
     for (unit = 0; unit < MAX_SCSI_DEVS; unit++) {
         dinfo = drive_get(IF_SCSI, bus->busnr, unit);
         if (dinfo == NULL) {
             continue;
         }
+        qemu_opts_loc_restore(dinfo->opts);
         if (!scsi_bus_legacy_add_drive(bus, dinfo->bdrv, unit)) {
             res = -1;
             break;
         }
     }
+    loc_pop(&loc);
     return res;
 }
 
-- 
1.6.6.1

[Qemu-devel] [PATCH v2 09/13] ide: Improve error messages

[Markus Armbruster]

Use error_report(), because it points to the error location.

Reword "tried to assign twice" messages to make it clear that we're
complaining about the unit property.

Report invalid unit property instead of failing silently.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/ide/qdev.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index 2977a16..221f387 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -18,7 +18,7 @@
  */
 #include <hw/hw.h>
 #include "dma.h"
-
+#include "qemu-error.h"
 #include <hw/ide/internal.h>
 
 /* --------------------------------- */
@@ -40,7 +40,7 @@ static int ide_qdev_init(DeviceState *qdev, DeviceInfo *base)
     IDEBus *bus = DO_UPCAST(IDEBus, qbus, qdev->parent_bus);
 
     if (!dev->conf.bs) {
-        fprintf(stderr, "%s: no drive specified\n", qdev->info->name);
+        error_report("No drive specified");
         goto err;
     }
     if (dev->unit == -1) {
@@ -49,19 +49,20 @@ static int ide_qdev_init(DeviceState *qdev, DeviceInfo *base)
     switch (dev->unit) {
     case 0:
         if (bus->master) {
-            fprintf(stderr, "ide: tried to assign master twice\n");
+            error_report("IDE unit %d is in use", dev->unit);
             goto err;
         }
         bus->master = dev;
         break;
     case 1:
         if (bus->slave) {
-            fprintf(stderr, "ide: tried to assign slave twice\n");
+            error_report("IDE unit %d is in use", dev->unit);
             goto err;
         }
         bus->slave = dev;
         break;
     default:
+        error_report("Invalid IDE unit %d", dev->unit);
         goto err;
     }
     return info->init(dev);
-- 
1.6.6.1

[Qemu-devel] Re: [PATCH v2 09/13] ide: Improve error messages

[Christoph Hellwig]

Looks good,


Reviewed-by: Christoph Hellwig <hch@lst.de>

[Qemu-devel] [PATCH v2 10/13] ide: Replace IDEState members is_cdrom, is_cf by drive_kind

[Markus Armbruster]

The two aren't independent variables.  Make that obvious.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/ide/core.c       |   40 ++++++++++++++++++++--------------------
 hw/ide/internal.h   |    5 +++--
 hw/ide/macio.c      |    2 +-
 hw/ide/microdrive.c |    2 +-
 4 files changed, 25 insertions(+), 24 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index ebdceb5..58b88ee 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -292,7 +292,7 @@ static void ide_set_signature(IDEState *s)
     /* put signature */
     s->nsector = 1;
     s->sector = 1;
-    if (s->is_cdrom) {
+    if (s->drive_kind == IDE_CD) {
         s->lcyl = 0x14;
         s->hcyl = 0xeb;
     } else if (s->bs) {
@@ -1827,15 +1827,15 @@ void ide_ioport_write(void *opaque, uint32_t addr, uint32_t val)
 
         switch(val) {
         case WIN_IDENTIFY:
-            if (s->bs && !s->is_cdrom) {
-                if (!s->is_cf)
+            if (s->bs && s->drive_kind != IDE_CD) {
+                if (s->drive_kind != IDE_CFATA)
                     ide_identify(s);
                 else
                     ide_cfata_identify(s);
                 s->status = READY_STAT | SEEK_STAT;
                 ide_transfer_start(s, s->io_buffer, 512, ide_transfer_stop);
             } else {
-                if (s->is_cdrom) {
+                if (s->drive_kind == IDE_CD) {
                     ide_set_signature(s);
                 }
                 ide_abort_command(s);
@@ -1849,7 +1849,7 @@ void ide_ioport_write(void *opaque, uint32_t addr, uint32_t val)
             ide_set_irq(s->bus);
             break;
         case WIN_SETMULT:
-            if (s->is_cf && s->nsector == 0) {
+            if (s->drive_kind == IDE_CFATA && s->nsector == 0) {
                 /* Disable Read and Write Multiple */
                 s->mult_sectors = 0;
                 s->status = READY_STAT | SEEK_STAT;
@@ -2033,7 +2033,7 @@ void ide_ioport_write(void *opaque, uint32_t addr, uint32_t val)
             ide_set_irq(s->bus);
             break;
         case WIN_SEEK:
-            if(s->is_cdrom)
+            if(s->drive_kind == IDE_CD)
                 goto abort_cmd;
             /* XXX: Check that seek is within bounds */
             s->status = READY_STAT | SEEK_STAT;
@@ -2041,7 +2041,7 @@ void ide_ioport_write(void *opaque, uint32_t addr, uint32_t val)
             break;
             /* ATAPI commands */
         case WIN_PIDENTIFY:
-            if (s->is_cdrom) {
+            if (s->drive_kind == IDE_CD) {
                 ide_atapi_identify(s);
                 s->status = READY_STAT | SEEK_STAT;
                 ide_transfer_start(s, s->io_buffer, 512, ide_transfer_stop);
@@ -2052,7 +2052,7 @@ void ide_ioport_write(void *opaque, uint32_t addr, uint32_t val)
             break;
         case WIN_DIAGNOSE:
             ide_set_signature(s);
-            if (s->is_cdrom)
+            if (s->drive_kind == IDE_CD)
                 s->status = 0; /* ATAPI spec (v6) section 9.10 defines packet
                                 * devices to return a clear status register
                                 * with READY_STAT *not* set. */
@@ -2064,14 +2064,14 @@ void ide_ioport_write(void *opaque, uint32_t addr, uint32_t val)
             ide_set_irq(s->bus);
             break;
         case WIN_SRST:
-            if (!s->is_cdrom)
+            if (s->drive_kind != IDE_CD)
                 goto abort_cmd;
             ide_set_signature(s);
             s->status = 0x00; /* NOTE: READY is _not_ set */
             s->error = 0x01;
             break;
         case WIN_PACKETCMD:
-            if (!s->is_cdrom)
+            if (s->drive_kind != IDE_CD)
                 goto abort_cmd;
             /* overlapping commands not supported */
             if (s->feature & 0x02)
@@ -2084,7 +2084,7 @@ void ide_ioport_write(void *opaque, uint32_t addr, uint32_t val)
             break;
         /* CF-ATA commands */
         case CFA_REQ_EXT_ERROR_CODE:
-            if (!s->is_cf)
+            if (s->drive_kind != IDE_CFATA)
                 goto abort_cmd;
             s->error = 0x09;    /* miscellaneous error */
             s->status = READY_STAT | SEEK_STAT;
@@ -2092,7 +2092,7 @@ void ide_ioport_write(void *opaque, uint32_t addr, uint32_t val)
             break;
         case CFA_ERASE_SECTORS:
         case CFA_WEAR_LEVEL:
-            if (!s->is_cf)
+            if (s->drive_kind != IDE_CFATA)
                 goto abort_cmd;
             if (val == CFA_WEAR_LEVEL)
                 s->nsector = 0;
@@ -2103,7 +2103,7 @@ void ide_ioport_write(void *opaque, uint32_t addr, uint32_t val)
             ide_set_irq(s->bus);
             break;
         case CFA_TRANSLATE_SECTOR:
-            if (!s->is_cf)
+            if (s->drive_kind != IDE_CFATA)
                 goto abort_cmd;
             s->error = 0x00;
             s->status = READY_STAT | SEEK_STAT;
@@ -2123,7 +2123,7 @@ void ide_ioport_write(void *opaque, uint32_t addr, uint32_t val)
             ide_set_irq(s->bus);
             break;
         case CFA_ACCESS_METADATA_STORAGE:
-            if (!s->is_cf)
+            if (s->drive_kind != IDE_CFATA)
                 goto abort_cmd;
             switch (s->feature) {
             case 0x02:	/* Inquiry Metadata Storage */
@@ -2143,7 +2143,7 @@ void ide_ioport_write(void *opaque, uint32_t addr, uint32_t val)
             ide_set_irq(s->bus);
             break;
         case IBM_SENSE_CONDITION:
-            if (!s->is_cf)
+            if (s->drive_kind != IDE_CFATA)
                 goto abort_cmd;
             switch (s->feature) {
             case 0x01:  /* sense temperature in device */
@@ -2157,7 +2157,7 @@ void ide_ioport_write(void *opaque, uint32_t addr, uint32_t val)
             break;
 
 	case WIN_SMART:
-	    if (s->is_cdrom)
+	    if (s->drive_kind == IDE_CD)
 		goto abort_cmd;
 	    if (s->hcyl != 0xc2 || s->lcyl != 0x4f)
 		goto abort_cmd;
@@ -2438,7 +2438,7 @@ void ide_cmd_write(void *opaque, uint32_t addr, uint32_t val)
         /* high to low */
         for(i = 0;i < 2; i++) {
             s = &bus->ifs[i];
-            if (s->is_cdrom)
+            if (s->drive_kind == IDE_CD)
                 s->status = 0x00; /* NOTE: READY is _not_ set */
             else
                 s->status = READY_STAT | SEEK_STAT;
@@ -2540,7 +2540,7 @@ static void ide_reset(IDEState *s)
 #ifdef DEBUG_IDE
     printf("ide: reset\n");
 #endif
-    if (s->is_cf)
+    if (s->drive_kind == IDE_CFATA)
         s->mult_sectors = 0;
     else
         s->mult_sectors = MAX_MULT_SECTORS;
@@ -2614,7 +2614,7 @@ void ide_init_drive(IDEState *s, BlockDriverState *bs,
     s->smart_errors = 0;
     s->smart_selftest_count = 0;
     if (bdrv_get_type_hint(bs) == BDRV_TYPE_CDROM) {
-        s->is_cdrom = 1;
+        s->drive_kind = IDE_CD;
         bdrv_set_change_cb(bs, cdrom_change_cb, s);
     }
     if (serial) {
@@ -2629,7 +2629,7 @@ void ide_init_drive(IDEState *s, BlockDriverState *bs,
         pstrcpy(s->version, sizeof(s->version), QEMU_VERSION);
     }
     ide_reset(s);
-    bdrv_set_removable(bs, s->is_cdrom);
+    bdrv_set_removable(bs, s->drive_kind == IDE_CD);
 }
 
 static void ide_init1(IDEBus *bus, int unit)
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 0125a9f..bd53660 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -362,6 +362,8 @@ typedef struct BMDMAState BMDMAState;
 #define SMART_DISABLE         0xd9
 #define SMART_STATUS          0xda
 
+typedef enum { IDE_HD, IDE_CD, IDE_CFATA } IDEDriveKind;
+
 typedef void EndTransferFunc(IDEState *);
 
 /* NOTE: IDEState represents in fact one drive */
@@ -369,8 +371,7 @@ struct IDEState {
     IDEBus *bus;
     uint8_t unit;
     /* ide config */
-    int is_cdrom;
-    int is_cf;
+    IDEDriveKind drive_kind;
     int cylinders, heads, sectors;
     int64_t nb_sectors;
     int mult_sectors;
diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index f76c0fa..539c067 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -162,7 +162,7 @@ static void pmac_ide_transfer(DBDMA_io *io)
     IDEState *s = idebus_active_if(&m->bus);
 
     s->io_buffer_size = 0;
-    if (s->is_cdrom) {
+    if (s->drive_kind == IDE_CD) {
         pmac_ide_atapi_transfer_cb(io, 0);
         return;
     }
diff --git a/hw/ide/microdrive.c b/hw/ide/microdrive.c
index a7beac5..47c3c8c 100644
--- a/hw/ide/microdrive.c
+++ b/hw/ide/microdrive.c
@@ -541,7 +541,7 @@ PCMCIACardState *dscm1xxxx_init(DriveInfo *bdrv)
 
     ide_init2_with_non_qdev_drives(&md->bus, bdrv, NULL,
                                    qemu_allocate_irqs(md_set_irq, md, 1)[0]);
-    md->bus.ifs[0].is_cf = 1;
+    md->bus.ifs[0].drive_kind = IDE_CFATA;
     md->bus.ifs[0].mdata_size = METADATA_SIZE;
     md->bus.ifs[0].mdata_storage = (uint8_t *) qemu_mallocz(METADATA_SIZE);
 
-- 
1.6.6.1

[Qemu-devel] Re: [PATCH v2 10/13] ide: Replace IDEState members is_cdrom, is_cf by drive_kind

[Christoph Hellwig]

I'd call it drive_type or just type, but either way the patch looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[Qemu-devel] [PATCH v2 11/13] ide: Make ide_init_drive() return success

[Markus Armbruster]

It still always succeeds.  The next commits will add failures.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/ide/core.c     |   13 +++++++++----
 hw/ide/internal.h |    4 ++--
 hw/ide/qdev.c     |    4 +++-
 3 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 58b88ee..dbb7acc 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -26,6 +26,7 @@
 #include <hw/pc.h>
 #include <hw/pci.h>
 #include <hw/scsi.h>
+#include "qemu-error.h"
 #include "qemu-timer.h"
 #include "sysemu.h"
 #include "dma.h"
@@ -2594,8 +2595,8 @@ void ide_bus_reset(IDEBus *bus)
     ide_clear_hob(bus);
 }
 
-void ide_init_drive(IDEState *s, BlockDriverState *bs,
-                    const char *version, const char *serial)
+int ide_init_drive(IDEState *s, BlockDriverState *bs,
+                   const char *version, const char *serial)
 {
     int cylinders, heads, secs;
     uint64_t nb_sectors;
@@ -2630,6 +2631,7 @@ void ide_init_drive(IDEState *s, BlockDriverState *bs,
     }
     ide_reset(s);
     bdrv_set_removable(bs, s->drive_kind == IDE_CD);
+    return 0;
 }
 
 static void ide_init1(IDEBus *bus, int unit)
@@ -2669,8 +2671,11 @@ void ide_init2_with_non_qdev_drives(IDEBus *bus, DriveInfo *hd0,
         dinfo = i == 0 ? hd0 : hd1;
         ide_init1(bus, i);
         if (dinfo) {
-            ide_init_drive(&bus->ifs[i], dinfo->bdrv, NULL,
-                           *dinfo->serial ? dinfo->serial : NULL);
+            if (ide_init_drive(&bus->ifs[i], dinfo->bdrv, NULL,
+                               *dinfo->serial ? dinfo->serial : NULL) < 0) {
+                error_report("Can't set up IDE drive %s", dinfo->id);
+                exit(1);
+            }
         } else {
             ide_reset(&bus->ifs[i]);
         }
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index bd53660..4165543 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -556,8 +556,8 @@ uint32_t ide_data_readw(void *opaque, uint32_t addr);
 void ide_data_writel(void *opaque, uint32_t addr, uint32_t val);
 uint32_t ide_data_readl(void *opaque, uint32_t addr);
 
-void ide_init_drive(IDEState *s, BlockDriverState *bs,
-                    const char *version, const char *serial);
+int ide_init_drive(IDEState *s, BlockDriverState *bs,
+                   const char *version, const char *serial);
 void ide_init2(IDEBus *bus, qemu_irq irq);
 void ide_init2_with_non_qdev_drives(IDEBus *bus, DriveInfo *hd0,
                                     DriveInfo *hd1, qemu_irq irq);
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index 221f387..53468ed 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -118,7 +118,9 @@ static int ide_drive_initfn(IDEDevice *dev)
         }
     }
 
-    ide_init_drive(s, dev->conf.bs, dev->version, serial);
+    if (ide_init_drive(s, dev->conf.bs, dev->version, serial) < 0) {
+        return -1;
+    }
 
     if (!dev->version) {
         dev->version = qemu_strdup(s->version);
-- 
1.6.6.1

[Qemu-devel] Re: [PATCH v2 11/13] ide: Make ide_init_drive() return success

[Christoph Hellwig]

Ok.

[Qemu-devel] [PATCH v2 12/13] ide: Reject readonly drives unless CD-ROM

[Markus Armbruster]

drive_init() doesn't permit option readonly for if=ide, but that's
worthless: we get it via if=none and -device.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/ide/core.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index dbb7acc..3b84bea 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2617,6 +2617,11 @@ int ide_init_drive(IDEState *s, BlockDriverState *bs,
     if (bdrv_get_type_hint(bs) == BDRV_TYPE_CDROM) {
         s->drive_kind = IDE_CD;
         bdrv_set_change_cb(bs, cdrom_change_cb, s);
+    } else {
+        if (bdrv_is_read_only(bs)) {
+            error_report("Can't use a read-only drive");
+            return -1;
+        }
     }
     if (serial) {
         strncpy(s->drive_serial_str, serial, sizeof(s->drive_serial_str));
-- 
1.6.6.1

[Qemu-devel] Re: [PATCH v2 12/13] ide: Reject readonly drives unless CD-ROM

[Christoph Hellwig]

Ok.

[Qemu-devel] [PATCH v2 13/13] ide: Reject invalid CHS geometry

[Markus Armbruster]

drive_init() doesn't permit invalid CHS for if=ide, but that's
worthless: we get it via if=none and -device.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/ide/core.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 3b84bea..af52c2c 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2604,6 +2604,18 @@ int ide_init_drive(IDEState *s, BlockDriverState *bs,
     s->bs = bs;
     bdrv_get_geometry(bs, &nb_sectors);
     bdrv_guess_geometry(bs, &cylinders, &heads, &secs);
+    if (cylinders < 1 || cylinders > 16383) {
+        error_report("cyls must be between 1 and 16383");
+        return -1;
+    }
+    if (heads < 1 || heads > 16) {
+        error_report("heads must be between 1 and 16");
+        return -1;
+    }
+    if (secs < 1 || secs > 63) {
+        error_report("secs must be between 1 and 63");
+        return -1;
+    }
     s->cylinders = cylinders;
     s->heads = heads;
     s->sectors = secs;
-- 
1.6.6.1

[Qemu-devel] Re: [PATCH v2 13/13] ide: Reject invalid CHS geometry

[Christoph Hellwig]

Looks good.

[Qemu-devel] Re: [PATCH v2 00/13] Still more block related fixes and cleanups

[Kevin Wolf]

Am 06.07.2010 14:08, schrieb Markus Armbruster:
> I'm working on cleanly separating block device host and guest parts.
> I'd like to route all this work through Kevin's block tree.  This is
> still just preliminaries.
> 
> This patch series is based available at
> git://repo.or.cz/qemu/armbru.git
> tag block-fixes-2-v2: this series, based on
> tag blockdev-base, which the current kevin/block
> 
> There will be at least one more round of cleanup & fixes before
> blockdev_add proper.  I intend to start with a minimal QMP-only
> version, then add features.
> 
> v2: rebased, better error messages (new 07/13 and 08/13)
> 
> Markus Armbruster (13):
>   blockdev: Clean up how readonly persists across virtual media change
>   block migration: Fix test for read-only drive
>   raw-posix: Fix test for host CD-ROM
>   fdc: Reject unimplemented error actions
>   qdev: Don't hw_error() in qdev_init_nofail()
>   scsi: Reject unimplemented error actions
>   error: New qemu_opts_loc_restore()
>   scsi: Error locations for -drive if=scsi device initialization
>   ide: Improve error messages
>   ide: Replace IDEState members is_cdrom, is_cf by drive_kind
>   ide: Make ide_init_drive() return success
>   ide: Reject readonly drives unless CD-ROM
>   ide: Reject invalid CHS geometry

Thanks, applied all to the block branch.

Kevin