From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43451) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZBG5t-00084C-6c for qemu-devel@nongnu.org; Sat, 04 Jul 2015 01:34:30 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZBG5r-0003Nu-Dv for qemu-devel@nongnu.org; Sat, 04 Jul 2015 01:34:29 -0400 Message-ID: <55977058.7070500@redhat.com> Date: Sat, 04 Jul 2015 01:34:16 -0400 From: John Snow MIME-Version: 1.0 References: <1435713640-12362-1-git-send-email-jsnow@redhat.com> <1435713640-12362-3-git-send-email-jsnow@redhat.com> <871tgp7523.fsf@blackfin.pond.sub.org> In-Reply-To: <871tgp7523.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC 02/10] fdc: add default drive type option List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: kwolf@redhat.com, qemu-devel@nongnu.org, qemu-block@nongnu.org On 07/03/2015 09:18 AM, Markus Armbruster wrote: > John Snow writes: > >> We want to change the current default drive type, >> but to be kind, we need to allow users to specify >> the old drive type somehow. > > Uh, what is *this* commit about? as far as I can tell, it adds drive > type properties (not a "default drive type option"), but doesn't put > them to use, yet. > Lost the refactor lottery and didn't update all of my initial commit messages. *ahem* Sorry. >> >> Signed-off-by: John Snow >> --- >> hw/block/fdc.c | 13 +++++++++++++ >> hw/core/qdev-properties.c | 12 ++++++++++++ >> include/hw/block/fdc.h | 7 +------ >> include/hw/qdev-properties.h | 1 + >> qapi/block.json | 15 +++++++++++++++ >> 5 files changed, 42 insertions(+), 6 deletions(-) >> >> diff --git a/hw/block/fdc.c b/hw/block/fdc.c >> index cdf9e09..1023a01 100644 >> --- a/hw/block/fdc.c >> +++ b/hw/block/fdc.c >> @@ -67,6 +67,8 @@ typedef struct FDFormat { >> FDriveRate rate; >> } FDFormat; >> >> +#define FDRIVE_DEFAULT FDRIVE_DRV_144 >> + >> static const FDFormat fd_formats[] = { >> /* First entry is default format */ >> /* 1.44 MB 3"1/2 floppy disks */ >> @@ -578,6 +580,9 @@ struct FDCtrl { >> /* Timers state */ >> uint8_t timer0; >> uint8_t timer1; >> + >> + FDriveType defaultA; >> + FDriveType defaultB; >> }; >> >> #define TYPE_SYSBUS_FDC "base-sysbus-fdc" >> @@ -2423,6 +2428,10 @@ static Property isa_fdc_properties[] = { >> DEFINE_PROP_DRIVE("driveB", FDCtrlISABus, state.drives[1].blk), >> DEFINE_PROP_BIT("check_media_rate", FDCtrlISABus, state.check_media_rate, >> 0, true), >> + DEFINE_PROP_DEFAULT("defaultA", FDCtrlISABus, state.defaultA, >> + FDRIVE_DEFAULT, qdev_prop_fdc_drive_type, FDriveType), >> + DEFINE_PROP_DEFAULT("defaultB", FDCtrlISABus, state.defaultB, >> + FDRIVE_DEFAULT, qdev_prop_fdc_drive_type, FDriveType), >> DEFINE_PROP_END_OF_LIST(), >> }; >> >> @@ -2471,6 +2480,10 @@ 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_DEFAULT("defaultA", FDCtrlSysBus, state.defaultA, >> + FDRIVE_DEFAULT, qdev_prop_fdc_drive_type, FDriveType), >> + DEFINE_PROP_DEFAULT("defaultB", FDCtrlSysBus, state.defaultB, >> + FDRIVE_DEFAULT, qdev_prop_fdc_drive_type, FDriveType), >> DEFINE_PROP_END_OF_LIST(), >> }; >> > > "defaultA" is not exactly a self-documenting name for a property > selecting the drive type. Even equally laconic "typeA" feels better. > >> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c >> index 47c1e8f..7ff8030 100644 >> --- a/hw/core/qdev-properties.c >> +++ b/hw/core/qdev-properties.c >> @@ -7,6 +7,7 @@ >> #include "net/hub.h" >> #include "qapi/visitor.h" >> #include "sysemu/char.h" >> +#include "hw/block/fdc.h" > > Why do you need to add the include? > ... Ghosts from iterations past. >> >> void qdev_prop_set_after_realize(DeviceState *dev, const char *name, >> Error **errp) >> @@ -543,6 +544,17 @@ PropertyInfo qdev_prop_bios_chs_trans = { >> .set = set_enum, >> }; >> >> +/* --- FDC default drive types */ >> + >> +PropertyInfo qdev_prop_fdc_drive_type = { >> + .name = "FdcDriveType", >> + .description = "FDC drive type, " >> + "none/120/144/288", >> + .enum_table = FDRIVE_DRV_lookup, >> + .get = get_enum, >> + .set = set_enum >> +}; >> + >> /* --- pci address --- */ >> >> /* >> diff --git a/include/hw/block/fdc.h b/include/hw/block/fdc.h >> index d48b2f8..f727027 100644 >> --- a/include/hw/block/fdc.h >> +++ b/include/hw/block/fdc.h >> @@ -6,12 +6,7 @@ >> /* fdc.c */ >> #define MAX_FD 2 >> >> -typedef enum FDriveType { >> - FDRIVE_DRV_144 = 0x00, /* 1.44 MB 3"5 drive */ >> - FDRIVE_DRV_288 = 0x01, /* 2.88 MB 3"5 drive */ >> - FDRIVE_DRV_120 = 0x02, /* 1.2 MB 5"25 drive */ >> - FDRIVE_DRV_NONE = 0x03, /* No drive connected */ >> -} FDriveType; >> +typedef FDRIVE_DRV FDriveType; > > See below. > >> >> #define TYPE_ISA_FDC "isa-fdc" >> >> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h >> index 0cfff1c..0872b41 100644 >> --- a/include/hw/qdev-properties.h >> +++ b/include/hw/qdev-properties.h >> @@ -20,6 +20,7 @@ extern PropertyInfo qdev_prop_ptr; >> extern PropertyInfo qdev_prop_macaddr; >> extern PropertyInfo qdev_prop_losttickpolicy; >> extern PropertyInfo qdev_prop_bios_chs_trans; >> +extern PropertyInfo qdev_prop_fdc_drive_type; >> extern PropertyInfo qdev_prop_drive; >> extern PropertyInfo qdev_prop_netdev; >> extern PropertyInfo qdev_prop_vlan; >> diff --git a/qapi/block.json b/qapi/block.json >> index aad645c..0c747a1 100644 >> --- a/qapi/block.json >> +++ b/qapi/block.json >> @@ -40,6 +40,21 @@ >> 'data': ['auto', 'none', 'lba', 'large', 'rechs']} >> >> ## >> +# FDRIVE_DRV: > > Stick in the usual @, please, just for consistency. > > FDRIVE_DRV is an unusual name for a QAPI type. I guess you chose it so > only the type name changes, but the enumeration constants stay the same. > You then hide away the type name change with a typedef in fdc.h. > > Clever, but I think in the longer run, I'd rather take a more > conventional type name. > If the rest of the series can be polished, I'll add a more traditional enum. This was more or less a quick PoC hack to avoid a lot of churn. >> +# >> +# Type of Floppy drive to be emulated by the Floppy Disk Controller. >> +# >> +# @144: 1.44MB 3.5" drive >> +# @288: 2.88MB 3.5" drive >> +# @120: 1.5MB 5.25" drive >> +# @none: No drive connected >> +# >> +# Since: 2.4 >> +## >> +{ 'enum': 'FDRIVE_DRV', >> + 'data': ['144', '288', '120', 'none']} >> + >> +## >> # @BlockdevSnapshotInternal >> # >> # @device: the name of the device to generate the snapshot from