Re: [Qemu-devel] [PATCH v5 3/4] fdc: rewrite seek and DSKCHG bit handling

[Kevin Wolf <kwolf@redhat.com>]

Am 13.06.2012 15:43, schrieb Pavel Hrdina:
> This bit is cleared on every successful seek to a different track (cylinder).
> The seek is also called on revalidate or on read/write/format commands which
> also clear the DSKCHG bit.
> 
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  hw/fdc.c |   68 +++++++++++++++++++++++++++++++-------------------------------
>  1 files changed, 34 insertions(+), 34 deletions(-)

Nice cleanup! Looks good generally, but this and patch 4 need test cases
as well before I can apply them.

> @@ -1004,30 +998,39 @@ static int fdctrl_seek_to_next_sect(FDCtrl *fdctrl, FDrive *cur_drv)
>                     fd_sector(cur_drv));
>      /* XXX: cur_drv->sect >= cur_drv->last_sect should be an
>         error in fact */
> -    if (cur_drv->sect >= cur_drv->last_sect ||
> -        cur_drv->sect == fdctrl->eot) {
> -        cur_drv->sect = 1;
> +    uint8_t new_head = cur_drv->head;
> +    uint8_t new_track = cur_drv->track;
> +    uint8_t new_sect = cur_drv->sect;
> +
> +    int ret = 1;
> +
> +    if (new_sect >= cur_drv->last_sect ||
> +        new_sect == fdctrl->eot) {
> +        new_sect = 1;
>          if (FD_MULTI_TRACK(fdctrl->data_state)) {
> -            if (cur_drv->head == 0 &&
> +            if (new_head == 0 &&
>                  (cur_drv->flags & FDISK_DBL_SIDES) != 0) {
> -                cur_drv->head = 1;
> +                new_head = 1;
>              } else {
> -                cur_drv->head = 0;
> -                cur_drv->track++;
> +                new_head = 0;
> +                new_track++;
>                  if ((cur_drv->flags & FDISK_DBL_SIDES) == 0)
> -                    return 0;
> +                    ret = 0;

Please use the chance to add braces here.

Can you also add a small header comment to the function that explains
what the return value means? Initially I read 0 as error, but in fact it
seems only to mean "request completed".

Kevin