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

[Markus Armbruster]

John Snow <address@hidden> 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.

>
> Signed-off-by: John Snow <address@hidden>
> ---
>  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?

>  
>  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.

> +#
> +# 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