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