On 11/03/2016 07:19, Pavel Dovgalyuk wrote: >> From: Paolo Bonzini [mailto:pbonz...@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