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

[Luiz Capitulino <lcapitulino@redhat.com>]

On Sat, 13 Sep 2014 19:27:46 +0300
Stratos Psomadakis <psomas@grnet.gr> wrote:

> On 12/09/2014 08:19 μμ, Luiz Capitulino wrote:
> > On Fri, 12 Sep 2014 20:01:04 +0300
> > Stratos Psomadakis <psomas@grnet.gr> wrote:
> >
> >> On 12/09/2014 06:21 μμ, 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 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?
> 
> 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.

> 
> >
> >> 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 event)
> >>>>          break;
> >>>>  
> >>>>      case CHR_EVENT_CLOSED:
> >>>> +        readline_restart(mon->rs);
> >>>>          mon_refcount--;
> >>>>          monitor_fdsets_cleanup();
> >>>>          break;
> >>
> 
>