[03/28] ide-test: add test for werror=stop

From: Paolo Bonzini <pbonzini@redhat.com>

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/ide-test.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 74 insertions(+)

On Mon, Jul 07, 2014 at 02:17:44PM -0400, John Snow wrote:
> @@ -489,6 +490,72 @@ static void test_flush(void)
>      ide_test_quit();
>  }
>  
> +static void prepare_blkdebug_script(const char *debug_path, const char *event)
> +{
> +    FILE *debug_file = fopen(debug_path, "w");

Please avoid shadowing the global debug_path variable.  Perhaps simply
call the argument 'filename'.

> +static void test_retry_flush(void)
> +{
> +    uint8_t data;
> +    const char *s;
> +
> +    prepare_blkdebug_script(debug_path, "flush_to_disk");
> +
> +    ide_test_start(
> +        "-vnc none "
> +        "-drive file=blkdebug:%s:%s,if=ide,cache=writeback,rerror=stop,werror=stop",
> +        debug_path, tmp_path);
> +
> +    /* FLUSH CACHE command on device 0*/
> +    outb(IDE_BASE + reg_device, 0);
> +    outb(IDE_BASE + reg_command, CMD_FLUSH_CACHE);
> +
> +    /* Check status while request is in flight*/
> +    data = inb(IDE_BASE + reg_status);
> +    assert_bit_set(data, BSY | DRDY);
> +    assert_bit_clear(data, DF | ERR | DRQ);
> +
> +    sleep(1);                    /* HACK: wait for event */
> +
> +    /* Complete the command */
> +    s = "{'execute':'cont' }";
> +    while (!qmp(s)) {
> +        s = "";
> +        sleep(1);
> +    }

I guess we're supposed to wait for the block I/O error event when the
machine stops.  Please implement that and replace this polling loop.

See the STOP event in docs/qmp/qmp-events.txt.

> @@ -501,6 +568,11 @@ int main(int argc, char **argv)
>          return 0;
>      }
>  
> +    /* Create temporary blkdebug instructions */
> +    fd = mkstemp(debug_path);
> +    g_assert(fd >= 0);
> +    close(fd);
> +

debug_path must be unlinked at the end of main().  Tests should not
clutter up the /tmp directory, all resources must be freed.

On 07/31/2014 06:58 AM, Stefan Hajnoczi wrote:
> On Mon, Jul 07, 2014 at 02:17:44PM -0400, John Snow wrote:
>> +static void test_retry_flush(void)
>> +{
>> +    uint8_t data;
>> +    const char *s;
>> +
>> +    prepare_blkdebug_script(debug_path, "flush_to_disk");
>> +
>> +    ide_test_start(
>> +        "-vnc none "
>> +        "-drive file=blkdebug:%s:%s,if=ide,cache=writeback,rerror=stop,werror=stop",
>> +        debug_path, tmp_path);
>> +
>> +    /* FLUSH CACHE command on device 0*/
>> +    outb(IDE_BASE + reg_device, 0);
>> +    outb(IDE_BASE + reg_command, CMD_FLUSH_CACHE);
>> +
>> +    /* Check status while request is in flight*/
>> +    data = inb(IDE_BASE + reg_status);
>> +    assert_bit_set(data, BSY | DRDY);
>> +    assert_bit_clear(data, DF | ERR | DRQ);
>> +
>> +    sleep(1);                    /* HACK: wait for event */
>> +
>> +    /* Complete the command */
>> +    s = "{'execute':'cont' }";
>> +    while (!qmp(s)) {
>> +        s = "";
>> +        sleep(1);
>> +    }
> I guess we're supposed to wait for the block I/O error event when the
> machine stops.  Please implement that and replace this polling loop.
>
> See the STOP event in docs/qmp/qmp-events.txt.
>
Paolo: I edited in a bit to check for the STOP event instead of sleeping 
in V2, but what's the point of setting s = "" and sleeping and resending 
a blank string? Can't we just g_assert(qmp(s)) the first go around, 
provided the STOP event has already occurred?

It works in practice, but I am curious.

--J

John Snow <jsnow@redhat.com> writes:

> On 07/31/2014 06:58 AM, Stefan Hajnoczi wrote:
>> On Mon, Jul 07, 2014 at 02:17:44PM -0400, John Snow wrote:
>>> +static void test_retry_flush(void)
>>> +{
>>> +    uint8_t data;
>>> +    const char *s;
>>> +
>>> +    prepare_blkdebug_script(debug_path, "flush_to_disk");
>>> +
>>> +    ide_test_start(
>>> +        "-vnc none "
>>> + "-drive
>>> file=blkdebug:%s:%s,if=ide,cache=writeback,rerror=stop,werror=stop",
>>> +        debug_path, tmp_path);
>>> +
>>> +    /* FLUSH CACHE command on device 0*/
>>> +    outb(IDE_BASE + reg_device, 0);
>>> +    outb(IDE_BASE + reg_command, CMD_FLUSH_CACHE);
>>> +
>>> +    /* Check status while request is in flight*/
>>> +    data = inb(IDE_BASE + reg_status);
>>> +    assert_bit_set(data, BSY | DRDY);
>>> +    assert_bit_clear(data, DF | ERR | DRQ);
>>> +
>>> +    sleep(1);                    /* HACK: wait for event */
>>> +
>>> +    /* Complete the command */
>>> +    s = "{'execute':'cont' }";
>>> +    while (!qmp(s)) {
>>> +        s = "";
>>> +        sleep(1);
>>> +    }
>> I guess we're supposed to wait for the block I/O error event when the
>> machine stops.  Please implement that and replace this polling loop.
>>
>> See the STOP event in docs/qmp/qmp-events.txt.
>>
> Paolo: I edited in a bit to check for the STOP event instead of
> sleeping in V2, but what's the point of setting s = "" and sleeping
> and resending a blank string? Can't we just g_assert(qmp(s)) the first
> go around, provided the STOP event has already occurred?
>
> It works in practice, but I am curious.

I don't understand the need for sleep.  qmp() & friends wait for a
reply.

Sending "", i.e. nothing, is a crude way to just receive a reply.

Example: boot-order-test.c waits for an event without examining it:

    qmp_discard_response("");   /* HACK: wait for event */

Your loop seems to try to consume replies (events?) until you get NULL,
which I guess happens when the other end closes the connection.

By the way, your loop leaks the responses.  Pretty harmless, but you may
want to QDECREF() just to be orderly.

Proper support for events in libqtest would be nice.