From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:60639)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <lcapitulino@redhat.com>) id 1XSyXw-00021V-SJ
	for qemu-devel@nongnu.org; Sat, 13 Sep 2014 21:24:13 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <lcapitulino@redhat.com>) id 1XSyXr-0004jz-ON
	for qemu-devel@nongnu.org; Sat, 13 Sep 2014 21:24:08 -0400
Date: Sat, 13 Sep 2014 21:23:49 -0400
From: Luiz Capitulino <lcapitulino@redhat.com>
Message-ID: <20140913212349.5fb6ca1d@redhat.com>
In-Reply-To: <54147082.5000904@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>
	<20140912131933.05b74346@redhat.com> <54147082.5000904@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 Sat, 13 Sep 2014 19:27:46 +0300
Stratos Psomadakis <psomas@grnet.gr> wrote:

> On 12/09/2014 08:19 =CE=BC=CE=BC, Luiz Capitulino wrote:
> > 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 cl=
oses the
> >>>> connection to the monitor, tcp_chr_read() will detect the G_IO_HUP c=
ondition
> >>>> and call tcp_chr_disconnect() to close the server-side connection to=
o. 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-fin=
ished
> >>>> command).  Thus, without calling readline_restart() on mon->rs upon
> >>>> CHR_EVENT_CLOSED, future HMP commands will fail.
> >>> What's your reproducer?
> >> 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 t=
he
> >> script (in the middle of the loop) and re-run it, we get "unknown
> >> command" errors from the HMP ("unknown command: 'infinfo'" for example=
).
> >>
> >>> Are you using the mux feature?
> >> Nope (on the cli we use '-monitor unix:<path>.mon,server,nowait' for t=
he
> >> HMP).
> >>
> >>> We also reset it
> >>> in CHR_EVENT_OPENED if the mux feature is not used, why isn't that
> >>> good enough?
> >> 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
> I did some quick tests with a mux chardev (I tried two mux'ed HMP
> monitors and a serial and an HMP). Calling readline_restart() in
> CHR_EVENT_CLOSED didn't seem to affect mux support (as far as I could
> tell). However, calling readline_restart() in CHR_EVENT_OPENED, just
> before readline_show_prompt(), resolves the issue too, and I think it
> makes more sense to be called at that point. If you agree, I can resend
> the modified patch.

Yes, I think that's the best. I'll just apply your respin.

>=20
> >
> >> Thanks,
> >> Stratos
> >>
> >>>> 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 ev=
ent)
> >>>>          break;
> >>>> =20
> >>>>      case CHR_EVENT_CLOSED:
> >>>> +        readline_restart(mon->rs);
> >>>>          mon_refcount--;
> >>>>          monitor_fdsets_cleanup();
> >>>>          break;
> >>
>=20
>=20