Re: [Qemu-block] [Qemu-devel] [PATCH v3 02/11] fdc: refactor pick_geometry

[Markus Armbruster]

John Snow <address@hidden> writes:

> On 12/17/2015 02:53 AM, Markus Armbruster wrote:
>> John Snow <address@hidden> writes:
>> 
>>> Modify this function to operate directly on FDrive objects instead of
>>> unpacking and passing all of those parameters manually.
>>>
>>> Helps reduce complexity in each caller, and reduces the number of args.
>> 
>> For now, there's just one.
>> 
>> Diffstat suggests it's an overall simplification.
>> 
>>> Signed-off-by: John Snow <address@hidden>
>>> ---
>>>  hw/block/fdc.c | 54 +++++++++++++++++++++++-------------------------------
>>>  1 file changed, 23 insertions(+), 31 deletions(-)
>>>
>>> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
>>> index 246b631..09bb63d 100644
>>> --- a/hw/block/fdc.c
>>> +++ b/hw/block/fdc.c
>>> @@ -241,11 +241,9 @@ static void fd_recalibrate(FDrive *drv)
>>>      fd_seek(drv, 0, 0, 1, 1);
>>>  }
>>>  
>>> -static void pick_geometry(BlockBackend *blk, int *nb_heads,
>>> -                          int *max_track, int *last_sect,
>>> -                          FDriveType drive_in, FDriveType *drive,
>>> -                          FDriveRate *rate)
>>> +static void pick_geometry(FDrive *drv)
>>>  {
>>> +    BlockBackend *blk = drv->blk;
>>>      const FDFormat *parse;
>>>      uint64_t nb_sectors, size;
>>>      int i, first_match, match;
>>> @@ -258,8 +256,8 @@ static void pick_geometry(BlockBackend *blk, int 
>>> *nb_heads,
>>>          if (parse->drive == FDRIVE_DRV_NONE) {
>>>              break;
>>>          }
>>> -        if (drive_in == parse->drive ||
>>> -            drive_in == FDRIVE_DRV_NONE) {
>>> +        if (drv->drive == parse->drive ||
>>> +            drv->drive == FDRIVE_DRV_NONE) {
>>>              size = (parse->max_head + 1) * parse->max_track *
>>>                  parse->last_sect;
>>>              if (nb_sectors == size) {
>>> @@ -279,41 +277,35 @@ static void pick_geometry(BlockBackend *blk, int 
>>> *nb_heads,
>>>          }
>>>          parse = &fd_formats[match];
>>>      }
>>> -    *nb_heads = parse->max_head + 1;
>>> -    *max_track = parse->max_track;
>>> -    *last_sect = parse->last_sect;
>>> -    *drive = parse->drive;
>>> -    *rate = parse->rate;
>>> +
>>> +    if (parse->max_head == 0) {
>>> +        drv->flags &= ~FDISK_DBL_SIDES;
>>> +    } else {
>>> +        drv->flags |= FDISK_DBL_SIDES;
>>> +    }
>>> +    drv->max_track = parse->max_track;
>>> +    drv->last_sect = parse->last_sect;
>>> +    drv->drive = parse->drive;
>>> +    drv->media_rate = parse->rate;
>>> +
>>> +    if (drv->media_inserted) {
>>> +        FLOPPY_DPRINTF("Floppy disk (%d h %d t %d s) %s\n",
>>> +                       parse->max_head + 1,
>>> +                       drv->max_track, drv->last_sect,
>>> +                       drv->ro ? "ro" : "rw");
>>> +    }
>> 
>> One half of the debug print moved from...
>> 
>>>  }
>>>  
>>>  /* Revalidate a disk drive after a disk change */
>>>  static void fd_revalidate(FDrive *drv)
>>>  {
>>> -    int nb_heads, max_track, last_sect, ro;
>>> -    FDriveType drive;
>>> -    FDriveRate rate;
>>> -
>>>      FLOPPY_DPRINTF("revalidate\n");
>>>      if (drv->blk != NULL) {
>>> -        ro = blk_is_read_only(drv->blk);
>>> -        pick_geometry(drv->blk, &nb_heads, &max_track,
>>> -                      &last_sect, drv->drive, &drive, &rate);
>>> +        drv->ro = blk_is_read_only(drv->blk);
>>> +        pick_geometry(drv);
>>>          if (!drv->media_inserted) {
>>>              FLOPPY_DPRINTF("No disk in drive\n");
>>> -        } else {
>>> -            FLOPPY_DPRINTF("Floppy disk (%d h %d t %d s) %s\n", nb_heads,
>>> -                           max_track, last_sect, ro ? "ro" : "rw");
>>>          }
>> 
>> ... here.  Recommend to move both or none.  If you add more callers,
>> both might make more sense.
>> 
>
> Later in the series, pick_geometry cannot be called if there's no
> diskette, so the "no disk!" message stayed outside.
>
> ... I'll just wiggle this printf back into fd_revalidate, so it looks sane.
>
> I encourage you to take a few shots of something stiff and press further
> into the series. It doesn't get truly wild until patch 9. If you
> eyeballed everything except patch 9, I'd be willing to just do a very
> early dev window pull and call it a day on this.

Just one day left before I vanish for a long Christas break.  I'll see
what I can do.