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

[Stratos Psomadakis]

On 12/09/2014 08:19 μμ, Luiz Capitulino wrote:
> 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?

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.

>
>> 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;
>>


-- 
Stratos Psomadakis
<address@hidden>

signature.asc
Description: OpenPGP digital signature