From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:60705)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <lcapitulino@redhat.com>) id 1XSUVk-0002EB-Hx
	for qemu-devel@nongnu.org; Fri, 12 Sep 2014 13:19:57 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <lcapitulino@redhat.com>) id 1XSUVf-0005UY-E3
	for qemu-devel@nongnu.org; Fri, 12 Sep 2014 13:19:52 -0400
Date: Fri, 12 Sep 2014 13:19:33 -0400
From: Luiz Capitulino <lcapitulino@redhat.com>
Message-ID: <20140912131933.05b74346@redhat.com>
In-Reply-To: <541326D0.4040102@grnet.gr>
References: <87wq991g1c.fsf@blackfin.pond.sub.org>
	<1410530852-13631-1-git-send-email-psomas@grnet.gr>
	<20140912112110.3b61c84b@redhat.com> <541326D0.4040102@grnet.gr>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Subject: Re: [Qemu-devel] [synnefo-devel] Re: [PATCH resend 1/2] monitor:
 Reset HMP mon->rs on CHR_EVENT_CLOSED
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: Stratos Psomadakis <psomas@grnet.gr>
Cc: synnefo-devel@googlegroups.com, qemu-devel@nongnu.org, armbru@redhat.com, qemu-stable@nongnu.org

On Fri, 12 Sep 2014 20:01:04 +0300
Stratos Psomadakis <psomas@grnet.gr> wrote:

> On 12/09/2014 06:21 =CE=BC=CE=BC, Luiz Capitulino wrote:
> > On Fri, 12 Sep 2014 17:07:32 +0300
> > Stratos Psomadakis <psomas@grnet.gr> wrote:
> >
> >> Commit cdaa86a54 ("Add G_IO_HUP handler for socket chardev") exposed a=
 bug in
> >> the way the HMP monitor handles its command buffer. When a client clos=
es the
> >> connection to the monitor, tcp_chr_read() will detect the G_IO_HUP con=
dition
> >> and call tcp_chr_disconnect() to close the server-side connection too.=
 Due to
> >> the fact that monitor reads 1 byte at a time (for each tcp_chr_read())=
, the
> >> monitor readline state / buffers might contain junk (i.e. a half-finis=
hed
> >> command).  Thus, without calling readline_restart() on mon->rs upon
> >> CHR_EVENT_CLOSED, future HMP commands will fail.
> > What's your reproducer?
>=20
> We have a script that opens a connection to the HMP socket and starts
> sending 'info version' commands to the monitor in a loop. If we kill the
> script (in the middle of the loop) and re-run it, we get "unknown
> command" errors from the HMP ("unknown command: 'infinfo'" for example).
>=20
> > Are you using the mux feature?
>=20
> Nope (on the cli we use '-monitor unix:<path>.mon,server,nowait' for the
> HMP).
>=20
> > We also reset it
> > in CHR_EVENT_OPENED if the mux feature is not used, why isn't that
> > good enough?
>=20
> I checked the code and on CHR_EVENT_OPENED the monitor calls
> readline_show_prompt (when not using mux). This resets the
> last_cmd_index/size readline variables, but the cmd_buf_index/size
> remains intact. I think that readline_restart() is necessary in order to
> cleanup the readline cmd buf (either in CHR_EVENT_OPENED or in
> CHR_EVENT_CLOSED).

I'm wondering if calling readline_restart() in the CHR_EVENT_CLOSED
can break mux support. But I won't have time to check it today. Maybe
moving the readline_restart() call to right before the
readline_show_prompt() call in the OPENED event is the best thing to do?

>=20
> Thanks,
> Stratos
>=20
> >
> >> Signed-off-by: Stratos Psomadakis <psomas@grnet.gr>
> >> Signed-off-by: Dimitris Aragiorgis <dimara@grnet.gr>
> >> ---
> >>  monitor.c |    1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/monitor.c b/monitor.c
> >> index 34cee74..7857300 100644
> >> --- a/monitor.c
> >> +++ b/monitor.c
> >> @@ -5252,6 +5252,7 @@ static void monitor_event(void *opaque, int even=
t)
> >>          break;
> >> =20
> >>      case CHR_EVENT_CLOSED:
> >> +        readline_restart(mon->rs);
> >>          mon_refcount--;
> >>          monitor_fdsets_cleanup();
> >>          break;
>=20
>=20