On 12/18/2015 11:11 AM, Markus Armbruster wrote: > John Snow <js...@redhat.com> writes: > >> Currently, fd_revalidate is called in two different places, with two >> different expectations of behavior: >> >> (1) On initialization, as a routine to help pick the drive type and >> initial geometries as a side-effect of the pick_geometry routine >> >> (2) On insert/eject, which either sets the geometries to a default value >> or chooses new geometries based on the inserted diskette. >> >> Break this nonsense apart by creating a new function dedicated towards >> picking the drive type on initialization. >> >> This has a few results: >> >> (1) fd_revalidate does not get called on boot anymore for drives with no >> diskette. >> >> (2) pick_geometry will actually get called twice if we have a diskette >> inserted, but this is harmless. (Once for the drive type, and once >> as part of the media callback.) >> >> Signed-off-by: John Snow <js...@redhat.com> >> --- >> hw/block/fdc.c | 36 +++++++++++++++++++++++++++++------- >> 1 file changed, 29 insertions(+), 7 deletions(-) >> >> diff --git a/hw/block/fdc.c b/hw/block/fdc.c >> index b587de8..e752758 100644 >> --- a/hw/block/fdc.c >> +++ b/hw/block/fdc.c >> @@ -167,6 +167,7 @@ static void fd_init(FDrive *drv) >> drv->disk = FLOPPY_DRIVE_TYPE_NONE; >> drv->last_sect = 0; >> drv->max_track = 0; >> + drv->ro = true; >> } >> >> #define NUM_SIDES(drv) ((drv)->flags & FDISK_DBL_SIDES ? 2 : 1) >> @@ -249,13 +250,21 @@ static void fd_recalibrate(FDrive *drv) >> fd_seek(drv, 0, 0, 1, 1); >> } >> >> -static void pick_geometry(FDrive *drv) >> +/** >> + * Determine geometry based on inserted diskette. >> + */ >> +static bool pick_geometry(FDrive *drv) > > Meaning of return value? >
1: "Modified diskette geometry values." Will amend with documentation. >> { >> BlockBackend *blk = drv->blk; >> const FDFormat *parse; >> uint64_t nb_sectors, size; >> int i, first_match, match; >> >> + /* We can only pick a geometry if we have a diskette. */ >> + if (!drv->media_inserted) { >> + return false; >> + } >> + >> blk_get_geometry(blk, &nb_sectors); >> match = -1; >> first_match = -1; >> @@ -293,8 +302,7 @@ static void pick_geometry(FDrive *drv) >> } >> drv->max_track = parse->max_track; >> drv->last_sect = parse->last_sect; >> - drv->drive = parse->drive; >> - drv->disk = drv->media_inserted ? parse->drive : FLOPPY_DRIVE_TYPE_NONE; >> + drv->disk = parse->drive; >> drv->media_rate = parse->rate; >> >> if (drv->media_inserted) { >> @@ -303,6 +311,14 @@ static void pick_geometry(FDrive *drv) >> drv->max_track, drv->last_sect, >> drv->ro ? "ro" : "rw"); >> } >> + return true; >> +} >> + >> +static void pick_drive_type(FDrive *drv) >> +{ >> + if (pick_geometry(drv)) { >> + drv->drive = drv->disk; >> + } >> } >> >> /* Revalidate a disk drive after a disk change */ >> @@ -311,15 +327,18 @@ static void fd_revalidate(FDrive *drv) >> FLOPPY_DPRINTF("revalidate\n"); >> if (drv->blk != NULL) { >> drv->ro = blk_is_read_only(drv->blk); >> - pick_geometry(drv); >> if (!drv->media_inserted) { >> FLOPPY_DPRINTF("No disk in drive\n"); >> + drv->disk = FLOPPY_DRIVE_TYPE_NONE; > > The left hand side is the "current disk type" (@disk's comment says so). > > The right hand side is "no drive connected" drive type (the enum's > comment says so). > > Thus, we set the current disk type to the "no drive connected" drive > type. Sounds nuts, doesn't it? :) > > Perhaps drv->disk isn't a disk type, but really into what an auto drive > type should be morphed. Correct? > ... yyyyes, it certainly informs us, if our type is auto, what we should be setting the drive type to. I'm definitely cheating, since there really should be two separate enumerations, honestly: DriveType: {120, 144, 288, auto, none} DiskType: {120, 144, 288, none} Though I concede the disk field isn't strictly needed, I was just desperate to not have the one field mean two things simultaneously. >> + } else { >> + pick_geometry(drv); >> } >> } else { >> FLOPPY_DPRINTF("No drive connected\n"); >> drv->last_sect = 0; >> drv->max_track = 0; >> drv->flags &= ~FDISK_DBL_SIDES; >> + drv->disk = FLOPPY_DRIVE_TYPE_NONE; >> } >> } >> >> @@ -2196,9 +2215,11 @@ static void fdctrl_change_cb(void *opaque, bool load) >> FDrive *drive = opaque; >> >> drive->media_inserted = load && drive->blk && >> blk_is_inserted(drive->blk); >> - >> drive->media_changed = 1; >> - fd_revalidate(drive); >> + >> + if (load) { >> + fd_revalidate(drive); >> + } > > Hmm. Looks like drv->last_sect, ->max_track and ->flags now remain > after an eject. If yes, why is that a good idea? > It probably isn't. Overlooked. I can allow the call to propagate as far as the revalidate function, since it won't call pick_geometry for empty drives anymore anyway. >> } >> >> static bool fdctrl_is_tray_open(void *opaque) >> @@ -2234,13 +2255,14 @@ static void fdctrl_connect_drives(FDCtrl *fdctrl, >> Error **errp) >> } >> >> fd_init(drive); >> - fdctrl_change_cb(drive, 0); >> if (drive->blk) { >> blk_set_dev_ops(drive->blk, &fdctrl_block_ops, drive); >> drive->media_inserted = blk_is_inserted(drive->blk); >> + pick_drive_type(drive); >> } else { >> drive->drive = FLOPPY_DRIVE_TYPE_NONE; >> } >> + fdctrl_change_cb(drive, drive->media_inserted); >> } >> }