Re: [Qemu-devel] [RFC 02/10] fdc: add default drive type option

[John Snow <jsnow@redhat.com>]

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