From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37763) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1adzdh-00011U-Iu for qemu-devel@nongnu.org; Thu, 10 Mar 2016 07:24:26 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1adzdc-0006fm-Jf for qemu-devel@nongnu.org; Thu, 10 Mar 2016 07:24:25 -0500 Received: from mx1.redhat.com ([209.132.183.28]:58066) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1adzdc-0006fb-E4 for qemu-devel@nongnu.org; Thu, 10 Mar 2016 07:24:20 -0500 References: <20160310115551.4812.55431.stgit@PASHA-ISP> <20160310115557.4812.55284.stgit@PASHA-ISP> From: Paolo Bonzini Message-ID: <56E1676D.9000703@redhat.com> Date: Thu, 10 Mar 2016 13:24:13 +0100 MIME-Version: 1.0 In-Reply-To: <20160310115557.4812.55284.stgit@PASHA-ISP> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v4 1/5] replay: character devices List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Pavel Dovgalyuk , qemu-devel@nongnu.org Cc: edgar.iglesias@xilinx.com, peter.maydell@linaro.org, igor.rubinov@gmail.com, mark.burton@greensocs.com, real@ispras.ru, batuzovk@ispras.ru, maria.klimushenkova@ispras.ru, stefanha@redhat.com, kwolf@redhat.com, hines@cert.org, alex.bennee@linaro.org, fred.konrad@greensocs.com On 10/03/2016 12:55, Pavel Dovgalyuk wrote: > gdbstub which also acts as a backend is not recorded to allow controlli= ng > the replaying through gdb. Perhaps the monitor too? 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 uin= t8_t *buf, int len) > qemu_chr_fe_write_log(s, buf, ret); > } > =20 > + if (s->replay) { > + replay_data_int(&ret); > + } I think this is wrong. The logic should be if (replaying) { read event(&ret); assert(ret <=3D len); len =3D ret; } qemu_mutex_lock(&s->chr_write_lock); ret =3D 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; > } > @@ -318,9 +322,19 @@ int qemu_chr_fe_read_all(CharDriverState *s, uint8= _t *buf, int len) > =20 > int qemu_chr_fe_ioctl(CharDriverState *s, int cmd, void *arg) > { > - if (!s->chr_ioctl) > - return -ENOTSUP; > - return s->chr_ioctl(s, cmd, arg); > + int res; > + if (!s->chr_ioctl) { > + res =3D -ENOTSUP; > + } else { > + res =3D s->chr_ioctl(s, cmd, arg); > + if (s->replay) { > + fprintf(stderr, > + "Replay: ioctl is not supported for serial devices= yet\n"); > + exit(1); Is it possible to print this warning just once per device and return -ENOTSUP instead? > +void replay_register_char_driver(CharDriverState *chr) > +{ > + if (replay_mode =3D=3D REPLAY_MODE_NONE) { > + return; > + } > + char_drivers =3D g_realloc(char_drivers, > + sizeof(*char_drivers) * (drivers_count + = 1)); > + char_drivers[drivers_count++] =3D chr; > +} You need a way to unregister character drivers when they are hot-unplugged, or at least you should block chardev-del if in record and replay mode. > + /* for int data */ > + EVENT_DATA_INT, I think you should call the event EVENT_CHAR_WRITE (and perhaps rename REPLAY_ASYNC_EVENT_CHAR to REPLAY_ASYNC_EVENT_CHAR_READ). And as mentioned above, I think the load and save cases should be separated in qemu-char.c, so I'd prefer to have a separate function to read and write the event as well. Thanks, Paolo