Re: [Qemu-block] [Qemu-devel] [PATCH v3 06/11] fdc: do not call revalidate on eject

[John Snow]

On 12/18/2015 11:11 AM, Markus Armbruster wrote:
> John Snow <address@hidden> 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 <address@hidden>
>> ---
>>  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);
>>      }
>>  }