Re: [Qemu-devel] [PATCH v6 5/6] fdc_test: update media_change test

[Kevin Wolf]

Am 22.06.2012 12:33, schrieb Pavel Hrdina:
> After rewrite DSKCHG bit handling the test has to be updated. Now
> is needed to seek to different track to clear DSKCHG bit.
> 
> Signed-off-by: Pavel Hrdina <address@hidden>
> ---
>  tests/fdc-test.c |   29 +++++++++++++++++++++--------
>  1 files changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/tests/fdc-test.c b/tests/fdc-test.c
> index 610e2f1..5280eff 100644
> --- a/tests/fdc-test.c
> +++ b/tests/fdc-test.c
> @@ -156,19 +156,20 @@ static uint8_t send_read_command(void)
>      return ret;
>  }
>  
> -static void send_step_pulse(void)
> +static void send_step_pulse(bool chg_cyl)
>  {
>      int drive = 0;
>      int head = 0;
> -    static int cyl = 0;
> +    int cyl = 0;
> +
> +    if (chg_cyl)
> +        cyl = (cyl + 1) % 4;

Why do you remove the static? Previously, the function would cycle
through cylinders 1-4. Now it's always 0 if !chg_cyl and 1 if chg_cyl. I
think you need to fix this.

I also don't quite understand why you move the increment to here instead
of leaving it as the bottom. It doesn't look wrong, but pointless.

>  
>      floppy_send(CMD_SEEK);
>      floppy_send(head << 2 | drive);
>      g_assert(!get_irq(FLOPPY_IRQ));
>      floppy_send(cyl);
>      ack_irq();
> -
> -    cyl = (cyl + 1) % 4;
>  }
>  
>  static uint8_t cmos_read(uint8_t reg)
> @@ -195,8 +196,7 @@ static void test_no_media_on_start(void)
>      assert_bit_set(dir, DSKCHG);
>      dir = inb(FLOPPY_BASE + reg_dir);
>      assert_bit_set(dir, DSKCHG);
> -    send_step_pulse();
> -    send_step_pulse();
> +    send_step_pulse(1);

This is a bool parameter, so you should pass true/false instead of 0/1.

Other than that, the test case additions look good and I'll accept it as
sufficient for the DSKCHG handling change. It would be great, though, to
add some more cases that test the implicit seeks during commands like
READ (ideally one for each command that can seek).

Kevin