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

[Qemu-devel] [PATCH 00/11] 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 on v2 of my previous series.
git://repo.or.cz/qemu/armbru.git
tag block-fixes-2-v1: this series, based on
tag block-fixes-v2: previous series, based on
tag blockdev-base, which is on branch origin/master

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.

Markus Armbruster (11):
  blockdev: Clean up how readonly persists across virtual media change
  block migration: Fix test for read-only drive
  raw-posix: Don't "try harder" for BDRV_TYPE_CDROM
  scsi: Reject unimplemented error actions
  fdc: Reject unimplemented error actions
  qdev: Don't hw_error() in qdev_init_nofail()
  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   |   26 ++----------------
 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-disk.c      |    5 +++
 hw/scsi-generic.c   |    9 ++++++
 12 files changed, 100 insertions(+), 68 deletions(-)

[Qemu-devel] [PATCH 01/11] 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] [PATCH 02/11] 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] [PATCH 03/11] raw-posix: Don't "try harder" for BDRV_TYPE_CDROM

[Markus Armbruster]

raw_pread_aligned() retries up to two times if the block device backs
a virtual CD-ROM.  This makes no sense.  Whether retrying reads can
correct read errors may depend on what we're reading, not on how the
result gets used.

Also clean up gratuitous use of goto.

This reverts what's left of commit 8c05dbf9.

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

diff --git a/block/raw-posix.c b/block/raw-posix.c
index 3f0701b..2a847aa 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;
         }
     }
 
@@ -259,23 +258,6 @@ static int raw_pread_aligned(BlockDriverState *bs, int64_t offset,
                       s->fd, bs->filename, offset, buf, count,
                       bs->total_sectors, ret, errno, strerror(errno));
 
-    /* Try harder for CDrom. */
-    if (bs->type == BDRV_TYPE_CDROM) {
-        ret = pread(s->fd, buf, count, offset);
-        if (ret == count)
-            goto label__raw_read__success;
-        ret = pread(s->fd, buf, count, offset);
-        if (ret == count)
-            goto label__raw_read__success;
-
-        DEBUG_BLOCK_PRINT("raw_pread(%d:%s, %" PRId64 ", %p, %d) [%" PRId64
-                          "] retry read failed %d : %d = %s\n",
-                          s->fd, bs->filename, offset, buf, count,
-                          bs->total_sectors, ret, errno, strerror(errno));
-    }
-
-label__raw_read__success:
-
     return  (ret < 0) ? -errno : ret;
 }
 
@@ -298,15 +280,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 03/11] raw-posix: Don't "try harder" for BDRV_TYPE_CDROM

[Kevin Wolf]

Am 30.06.2010 13:55, schrieb Markus Armbruster:
> raw_pread_aligned() retries up to two times if the block device backs
> a virtual CD-ROM.  This makes no sense.  Whether retrying reads can
> correct read errors may depend on what we're reading, not on how the
> result gets used.
> 
> Also clean up gratuitous use of goto.
> 
> This reverts what's left of commit 8c05dbf9.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Are you sure that this won't cause a regression? I mean if there is a
patch specifically adding this behaviour, there probably was a problem
that made someone touch the code in the first place.

Arguably checking for the type hint is nonsense, however I think the
case for which this was written is passing through a real CD-ROM to a VM
- in which case the condition would be true anyway.

So instead of removing the code, the fix to achieve what was probably
intended is to check for bs->drv == &bdrv_host_cdrom.

Kevin

[Qemu-devel] Re: [PATCH 03/11] raw-posix: Don't "try harder" for BDRV_TYPE_CDROM

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 30.06.2010 13:55, schrieb Markus Armbruster:
>> raw_pread_aligned() retries up to two times if the block device backs
>> a virtual CD-ROM.  This makes no sense.  Whether retrying reads can
>> correct read errors may depend on what we're reading, not on how the
>> result gets used.
>> 
>> Also clean up gratuitous use of goto.
>> 
>> This reverts what's left of commit 8c05dbf9.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> Are you sure that this won't cause a regression? I mean if there is a
> patch specifically adding this behaviour, there probably was a problem
> that made someone touch the code in the first place.
>
> Arguably checking for the type hint is nonsense, however I think the
> case for which this was written is passing through a real CD-ROM to a VM
> - in which case the condition would be true anyway.
>
> So instead of removing the code, the fix to achieve what was probably
> intended is to check for bs->drv == &bdrv_host_cdrom.

I can do that.  But does it make sense?  How can retrying failed reads
help?  Isn't the OS in a much better position to retry?

Keeping the retry code feels like voodoo-programming to me: I have no
idea how waving around this dead chicken could help, but we've always
done it, so keep waving ;)

[Qemu-devel] Re: [PATCH 03/11] raw-posix: Don't "try harder" for BDRV_TYPE_CDROM

[Kevin Wolf]

Am 05.07.2010 18:15, schrieb Markus Armbruster:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
>> Am 30.06.2010 13:55, schrieb Markus Armbruster:
>>> raw_pread_aligned() retries up to two times if the block device backs
>>> a virtual CD-ROM.  This makes no sense.  Whether retrying reads can
>>> correct read errors may depend on what we're reading, not on how the
>>> result gets used.
>>>
>>> Also clean up gratuitous use of goto.
>>>
>>> This reverts what's left of commit 8c05dbf9.
>>>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>
>> Are you sure that this won't cause a regression? I mean if there is a
>> patch specifically adding this behaviour, there probably was a problem
>> that made someone touch the code in the first place.
>>
>> Arguably checking for the type hint is nonsense, however I think the
>> case for which this was written is passing through a real CD-ROM to a VM
>> - in which case the condition would be true anyway.
>>
>> So instead of removing the code, the fix to achieve what was probably
>> intended is to check for bs->drv == &bdrv_host_cdrom.
> 
> I can do that.  But does it make sense?  How can retrying failed reads
> help?  Isn't the OS in a much better position to retry?
> 
> Keeping the retry code feels like voodoo-programming to me: I have no
> idea how waving around this dead chicken could help, but we've always
> done it, so keep waving ;)

I would agree that someone tried to be clever without real reason if
this was buried in one of those big Fabrice-style commits. But is was
added as a commit for itself, and I'd be surprised if someone sent a
patch if it didn't change anything for him.

Let's try if I've got a valid email address of Ben for CCing him... Ben,
do you remember this patch and can you help us?

commit 8c05dbf9b68cc8444573116063582e01a0442b0b
Author: ths <ths@c046a42c-6fe2-441c-8c8c-71466251a162>
Date:   Thu Sep 13 12:29:23 2007 +0000

    Enhance raw io reliability, by Ben Guthro.

Kevin

[Qemu-devel] [PATCH 04/11] 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.

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 04/11] scsi: Reject unimplemented error actions

[Kevin Wolf]

Am 30.06.2010 13:55, schrieb 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.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

It's still not entirely right, of course. You can explicitly request
werror=enospc and the VM will start successfully, but silently ignore
ENOSPC errors.

Anyway, it's better than before.

Kevin

[Qemu-devel] Re: [PATCH 04/11] scsi: Reject unimplemented error actions

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 30.06.2010 13:55, schrieb 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.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> It's still not entirely right, of course. You can explicitly request
> werror=enospc and the VM will start successfully, but silently ignore
> ENOSPC errors.

Yes, I took a shortcut here :)

Obvious fix is a separate default action.  Drivers supporting werror
interpret it as "enospc".  Drivers not supporting werror interpret it as
"do nothing", and reject any other option.  Similar for rerror.

> Anyway, it's better than before.

[Qemu-devel] [PATCH 05/11] 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.

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] [PATCH 06/11] 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] [PATCH 07/11] 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] [PATCH 08/11] 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..c37897b 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_CF)
                     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_CF && 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_CF)
                 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_CF)
                 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_CF)
                 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_CF)
                 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_CF)
                 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_CF)
         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..14b0035 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_CF } 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..4dd7419 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_CF;
     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 08/11] ide: Replace IDEState members is_cdrom, is_cf by drive_kind

[Kevin Wolf]

Am 30.06.2010 13:55, schrieb Markus Armbruster:
> The two aren't independent variables.  Make that obvious.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Can we please call the constant IDE_CDROM or something else that is easy
to distinguish? With this patch, we have IDE_CD and IDE_CF, which is an
easy typo to make.

Kevin

[Qemu-devel] Re: [PATCH 08/11] ide: Replace IDEState members is_cdrom, is_cf by drive_kind

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 30.06.2010 13:55, schrieb Markus Armbruster:
>> The two aren't independent variables.  Make that obvious.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> Can we please call the constant IDE_CDROM or something else that is easy
> to distinguish? With this patch, we have IDE_CD and IDE_CF, which is an
> easy typo to make.

I renamed IDE_CF to IDE_CFATA.  Hope that's okay.

[Qemu-devel] Re: [PATCH 08/11] ide: Replace IDEState members is_cdrom, is_cf by drive_kind

[Kevin Wolf]

Am 06.07.2010 14:38, schrieb Markus Armbruster:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
>> Am 30.06.2010 13:55, schrieb Markus Armbruster:
>>> The two aren't independent variables.  Make that obvious.
>>>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>
>> Can we please call the constant IDE_CDROM or something else that is easy
>> to distinguish? With this patch, we have IDE_CD and IDE_CF, which is an
>> easy typo to make.
> 
> I renamed IDE_CF to IDE_CFATA.  Hope that's okay.

Works for me.

Kevin

[Qemu-devel] [PATCH 09/11] 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 c37897b..a0eb1fa 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 14b0035..921856e 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] [PATCH 10/11] 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 a0eb1fa..73eae20 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] [PATCH 11/11] 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 73eae20..11edca5 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 00/11] Still more block related fixes and cleanups

[Kevin Wolf]

Am 30.06.2010 13:55, 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 on v2 of my previous series.
> git://repo.or.cz/qemu/armbru.git
> tag block-fixes-2-v1: this series, based on
> tag block-fixes-v2: previous series, based on
> tag blockdev-base, which is on branch origin/master
> 
> 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.
> 
> Markus Armbruster (11):
>   blockdev: Clean up how readonly persists across virtual media change
>   block migration: Fix test for read-only drive
>   raw-posix: Don't "try harder" for BDRV_TYPE_CDROM
>   scsi: Reject unimplemented error actions
>   fdc: Reject unimplemented error actions
>   qdev: Don't hw_error() in qdev_init_nofail()
>   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

I sent comments where I had some, the rest looks okay.

Kevin

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

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 30.06.2010 13:55, 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 on v2 of my previous series.
>> git://repo.or.cz/qemu/armbru.git
>> tag block-fixes-2-v1: this series, based on
>> tag block-fixes-v2: previous series, based on
>> tag blockdev-base, which is on branch origin/master
>> 
>> 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.
>> 
>> Markus Armbruster (11):
>>   blockdev: Clean up how readonly persists across virtual media change
>>   block migration: Fix test for read-only drive
>>   raw-posix: Don't "try harder" for BDRV_TYPE_CDROM
>>   scsi: Reject unimplemented error actions
>>   fdc: Reject unimplemented error actions
>>   qdev: Don't hw_error() in qdev_init_nofail()
>>   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
>
> I sent comments where I had some, the rest looks okay.

I'll work in your suggestions and respin.

Thanks!