From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55769) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a9TNz-0007rl-TS for qemu-devel@nongnu.org; Thu, 17 Dec 2015 02:54:05 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a9TNy-0001GB-Mb for qemu-devel@nongnu.org; Thu, 17 Dec 2015 02:54:03 -0500 From: Markus Armbruster References: <1450304177-3935-1-git-send-email-jsnow@redhat.com> <1450304177-3935-3-git-send-email-jsnow@redhat.com> Date: Thu, 17 Dec 2015 08:53:54 +0100 In-Reply-To: <1450304177-3935-3-git-send-email-jsnow@redhat.com> (John Snow's message of "Wed, 16 Dec 2015 17:16:08 -0500") Message-ID: <87oadp1pf1.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v3 02/11] fdc: refactor pick_geometry List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: John Snow Cc: kwolf@redhat.com, qemu-devel@nongnu.org, qemu-block@nongnu.org John Snow 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 > --- > 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. > - if (nb_heads == 1) { > - drv->flags &= ~FDISK_DBL_SIDES; > - } else { > - drv->flags |= FDISK_DBL_SIDES; > - } > - drv->max_track = max_track; > - drv->last_sect = last_sect; > - drv->ro = ro; > - drv->drive = drive; > - drv->media_rate = rate; > } else { > FLOPPY_DPRINTF("No drive connected\n"); > drv->last_sect = 0;