On 07/03/2015 09:18 AM, Markus Armbruster wrote:
> John Snow <js...@redhat.com> 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 <js...@redhat.com>
>> ---
>> 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
