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

[Markus Armbruster]

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?

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

> +        } 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?

>  }
>  
>  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);
>      }
>  }