| Message ID | 1435713640-12362-3-git-send-email-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
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. > > 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? > > 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
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. > > 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(), > }; > You missed sun4m_fdc_properties[]. [...]
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
John Snow <jsnow@redhat.com> writes: > On 07/03/2015 09:18 AM, Markus Armbruster wrote: [...] >>> 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. Makes sense, actually :) [...]
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(), }; 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" 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; #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: +# +# 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
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. 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(-)