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

[Pavel Dovgalyuk]

> From: Paolo Bonzini [mailto:address@hidden
> 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);
...
}


Pavel Dovgalyuk