Re: [Qemu-devel] [PATCH v4 1/5] replay: character devices

[Paolo Bonzini <pbonzini@redhat.com>]

On 11/03/2016 07:19, Pavel Dovgalyuk wrote:
>> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
>> On 10/03/2016 12:55, Pavel Dovgalyuk wrote:
>>> gdbstub which also acts as a backend is not recorded to allow controlling
>>> the replaying through gdb.
>>
>> Perhaps the monitor too?
> 
> Right. I'll check that it works.
> 
>> Overall the patch is nice and can definitely go in 2.6, but there are a
>> couple changes to do...
>>
>>> @@ -245,6 +246,9 @@ int qemu_chr_fe_write(CharDriverState *s, const uint8_t *buf, int len)
>>>          qemu_chr_fe_write_log(s, buf, ret);
>>>      }
>>>
>>> +    if (s->replay) {
>>> +        replay_data_int(&ret);
>>> +    }
>>
>> I think this is wrong.  The logic should be
>>
>>     if (replaying) {
>> 	read event(&ret);
>> 	assert(ret <= len);
>> 	len = ret;
>>     }
>>
>>     qemu_mutex_lock(&s->chr_write_lock);
>>     ret = s->chr_write(s, buf, len);
>>
>>     if (ret > 0) {
>>         qemu_chr_fe_write_log(s, buf, ret);
>>     }
>>     qemu_mutex_unlock(&s->chr_write_lock);
>>
>>     if (recording) {
>>         write event(ret);
>>     }
>>
>>>      qemu_mutex_unlock(&s->chr_write_lock);
>>>      return ret;
> 
> In this case return value in record and replay modes may differ
> and the behavior of caller won't be deterministic.
> E.g.,
> 
> static gboolean cadence_uart_xmit(GIOChannel *chan, GIOCondition cond,
>                                   void *opaque)
> {
> ...
>     ret = qemu_chr_fe_write(s->chr, s->tx_fifo, s->tx_count);
>     s->tx_count -= ret;
>     memmove(s->tx_fifo, s->tx_fifo + ret, s->tx_count);
> ...
> }

What you are doing is actually worse.  Say you are writing 20 bytes, and
at recording time the chardev could only write 10.  At replay time, you
will write 20 but perhaps the chardev (which is an external program,
remember) this time could write 15.  Now you have written 15 characters,
but you tell the device model that you have written 10.  The result is
that you'll write the 11th to 15th characters twice.

Likewise you could lose characters if the chardev cannot satisfy the
write at replay time.  With my version the latter is still possible, but
duplicated characters are not.

So perhaps:

    if (replaying) {
        read event(&ret);
        assert(ret <= len);
        ret = qemu_chr_fe_write_all(s, buf, ret);
        return ret;
    }

    qemu_mutex_lock(&s->chr_write_lock);
    ret = s->chr_write(s, buf, len);

    if (ret > 0) {
        qemu_chr_fe_write_log(s, buf, ret);
    }
    qemu_mutex_unlock(&s->chr_write_lock);
    if (recording) {
        write event(ret);
    }
    return ret;

Paolo