From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:50774)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <dovgaluk@ispras.ru>) id 1aeGQT-0003M7-Sc
	for qemu-devel@nongnu.org; Fri, 11 Mar 2016 01:19:54 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <dovgaluk@ispras.ru>) id 1aeGQQ-0007oS-Jk
	for qemu-devel@nongnu.org; Fri, 11 Mar 2016 01:19:53 -0500
Received: from mail.ispras.ru ([83.149.199.45]:54554)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <dovgaluk@ispras.ru>) id 1aeGQQ-0007oI-Bp
	for qemu-devel@nongnu.org; Fri, 11 Mar 2016 01:19:50 -0500
From: "Pavel Dovgalyuk" <dovgaluk@ispras.ru>
References: <20160310115551.4812.55431.stgit@PASHA-ISP>
	<20160310115557.4812.55284.stgit@PASHA-ISP>
	<56E1676D.9000703@redhat.com>
In-Reply-To: <56E1676D.9000703@redhat.com>
Date: Fri, 11 Mar 2016 09:19:42 +0300
Message-ID: <000c01d17b5e$01123dd0$0336b970$@ru>
MIME-Version: 1.0
Content-Type: text/plain;
	charset="utf-8"
Content-Transfer-Encoding: 7bit
Content-Language: ru
Subject: Re: [Qemu-devel] [PATCH v4 1/5] replay: character devices
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: 'Paolo Bonzini' <pbonzini@redhat.com>, 'Pavel Dovgalyuk' <Pavel.Dovgaluk@ispras.ru>, 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

> 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);
...
}


Pavel Dovgalyuk