John Snow <js...@redhat.com> writes:
> On 12/17/2015 02:53 AM, Markus Armbruster wrote:
>> John Snow <js...@redhat.com> 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 <js...@redhat.com>
>>> ---
>>> 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.
