Re: [Qemu-devel] [synnefo-devel] Re: [PATCH resend 1/2] monitor: Reset HMP mon->rs on CHR_EVENT_CLOSED

[Luiz Capitulino]

On Fri, 12 Sep 2014 20:01:04 +0300
Stratos Psomadakis <address@hidden> wrote:

> On 12/09/2014 06:21 μμ, Luiz Capitulino wrote:
> > On Fri, 12 Sep 2014 17:07:32 +0300
> > Stratos Psomadakis <address@hidden> 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 closes 
> >> the
> >> connection to the monitor, tcp_chr_read() will detect the G_IO_HUP 
> >> condition
> >> 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-finished
> >> 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 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).
> 
> > Are you using the mux feature?
> 
> Nope (on the cli we use '-monitor unix:<path>.mon,server,nowait' for the
> 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?

> 
> Thanks,
> Stratos
> 
> >
> >> Signed-off-by: Stratos Psomadakis <address@hidden>
> >> Signed-off-by: Dimitris Aragiorgis <address@hidden>
> >> ---
> >>  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 event)
> >>          break;
> >>  
> >>      case CHR_EVENT_CLOSED:
> >> +        readline_restart(mon->rs);
> >>          mon_refcount--;
> >>          monitor_fdsets_cleanup();
> >>          break;
> 
>