From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34403) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aA1PF-0003r8-Os for qemu-devel@nongnu.org; Fri, 18 Dec 2015 15:13:38 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aA1PE-000199-HI for qemu-devel@nongnu.org; Fri, 18 Dec 2015 15:13:37 -0500 References: <1450304177-3935-1-git-send-email-jsnow@redhat.com> <1450304177-3935-7-git-send-email-jsnow@redhat.com> <87y4cr3ff2.fsf@blackfin.pond.sub.org> From: John Snow Message-ID: <567468E9.1000203@redhat.com> Date: Fri, 18 Dec 2015 15:13:29 -0500 MIME-Version: 1.0 In-Reply-To: <87y4cr3ff2.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 06/11] fdc: do not call revalidate on eject List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: kwolf@redhat.com, qemu-devel@nongnu.org, qemu-block@nongnu.org On 12/18/2015 11:11 AM, Markus Armbruster wrote: > John Snow 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 >> --- >> 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); >> } >> }