Re: [Qemu-devel] [PATCH v2 3/4] fdc: Move qdev properties to FloppyDrive

[Kevin Wolf <kwolf@redhat.com>]

Am 15.10.2016 um 00:32 hat John Snow geschrieben:
> On 09/30/2016 03:39 PM, Kevin Wolf wrote:
> >This makes the FloppyDrive qdev object actually useful: Now that it has
> >all properties that don't belong to the controller, you can actually
> >use '-device floppy' and get a working result.
> >
> >Command line semantics is consistent with CD-ROM drives: By default you
> >get a single empty floppy drive. You can override it with -drive and
> >using the same index, but if you use -drive to add a floppy to a
> >different index, you get both of them. However, as soon as you use any
> >'-device floppy', even to a different slot, the default drive is
> >disabled.
> >
> >Using '-device floppy' without specifying the unit will choose the first
> >free slot on the controller.
> >
> >Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> >---
> > hw/block/fdc.c | 112 ++++++++++++++++++++++++++++++++++++++++++---------------
> > vl.c           |   1 +
> > 2 files changed, 85 insertions(+), 28 deletions(-)
> >
> >diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> >index 5aa8e52..00c0ec6 100644
> >--- a/hw/block/fdc.c
> >+++ b/hw/block/fdc.c
> >@@ -35,6 +35,7 @@
> > #include "qemu/timer.h"
> > #include "hw/isa/isa.h"
> > #include "hw/sysbus.h"
> >+#include "hw/block/block.h"
> > #include "sysemu/block-backend.h"
> > #include "sysemu/blockdev.h"
> > #include "sysemu/sysemu.h"
> >@@ -487,12 +488,18 @@ static const BlockDevOps fd_block_ops = {
> >      OBJECT_CHECK(FloppyDrive, (obj), TYPE_FLOPPY_DRIVE)
> >
> > typedef struct FloppyDrive {
> >-    DeviceState qdev;
> >-    uint32_t    unit;
> >+    DeviceState     qdev;
> >+    uint32_t        unit;
> >+    BlockConf       conf;
> >+    FloppyDriveType type;
> > } FloppyDrive;
> >
> > static Property floppy_drive_properties[] = {
> >     DEFINE_PROP_UINT32("unit", FloppyDrive, unit, -1),
> >+    DEFINE_BLOCK_PROPERTIES(FloppyDrive, conf),
> >+    DEFINE_PROP_DEFAULT("drive-type", FloppyDrive, type,
> >+                        FLOPPY_DRIVE_TYPE_AUTO, qdev_prop_fdc_drive_type,
> >+                        FloppyDriveType),
> >     DEFINE_PROP_END_OF_LIST(),
> > };
> >
> >@@ -501,6 +508,7 @@ static int floppy_drive_init(DeviceState *qdev)
> >     FloppyDrive *dev = FLOPPY_DRIVE(qdev);
> >     FloppyBus *bus = DO_UPCAST(FloppyBus, bus, dev->qdev.parent_bus);
> >     FDrive *drive;
> >+    int ret;
> >
> >     if (dev->unit == -1) {
> >         for (dev->unit = 0; dev->unit < MAX_FD; dev->unit++) {
> >@@ -517,29 +525,57 @@ static int floppy_drive_init(DeviceState *qdev)
> >         return -1;
> >     }
> >
> >-    /* TODO Check whether unit is in use */
> >-
> >     drive = get_drv(bus->fdc, dev->unit);
> >-
> >     if (drive->blk) {
> >-        if (blk_get_on_error(drive->blk, 0) != BLOCKDEV_ON_ERROR_ENOSPC) {
> >-            error_report("fdc doesn't support drive option werror");
> >-            return -1;
> >-        }
> >-        if (blk_get_on_error(drive->blk, 1) != BLOCKDEV_ON_ERROR_REPORT) {
> >-            error_report("fdc doesn't support drive option rerror");
> >-            return -1;
> >-        }
> >-    } else {
> >+        error_report("Floppy unit %d is in use", dev->unit);
> >+        return -1;
> >+    }
> >+
> >+    if (!dev->conf.blk) {
> >         /* Anonymous BlockBackend for an empty drive */
> >-        drive->blk = blk_new();
> >+        dev->conf.blk = blk_new();
> >+        ret = blk_attach_dev(dev->conf.blk, dev);
> 
> Missing a 'q' here:                           ^

Yes. It has the same value, but after my last pull request we need a
DeviceState* here indeed rather than a void*.

> >@@ -2782,8 +2838,8 @@ static const VMStateDescription vmstate_sysbus_fdc ={
> > };
> >
> > static Property sysbus_fdc_properties[] = {
> >-    DEFINE_PROP_DRIVE("driveA", FDCtrlSysBus, state.drives[0].blk),
> >-    DEFINE_PROP_DRIVE("driveB", FDCtrlSysBus, state.drives[1].blk),
> >+    DEFINE_PROP_DRIVE("driveA", FDCtrlSysBus, state.qdev_for_drives[0].blk),
> >+    DEFINE_PROP_DRIVE("driveB", FDCtrlSysBus, state.qdev_for_drives[1].blk),
> >     DEFINE_PROP_DEFAULT("fdtypeA", FDCtrlSysBus, state.drives[0].drive,
> >                         FLOPPY_DRIVE_TYPE_AUTO, qdev_prop_fdc_drive_type,
> >                         FloppyDriveType),
> 
> ^ Does sysbus' type property not need updating ...?

Doing half of the properties here felt like a good transitional step
from fully converting the PC device to completely ignoring Sun.

Well, I guess, I should fix that...

Kevin