Re: [Qemu-devel] [PATCH 03/28] ide-test: add test for werror=stop

[Markus Armbruster]

John Snow <address@hidden> 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.