[Qemu-devel] [PATCH 0/2] Monitor-related fixes

[Qemu-devel] [PATCH 0/2] Monitor-related fixes

[Stratos Psomadakis]

Hi,

the first patch fixes an issue with HMP monitors, which was exposed with v2.1.0
(commits cdaa86a and 812c10). The second one fixes a typo in a helper C program
used in qemu-iotests.

We think that they should be cherry-picked for the next stable release.

Thanks,
Stratos

Stratos Psomadakis (2):
  monitor: Reset HMP mon->rs on CHR_EVENT_CLOSED
  iotests: Send the correct fd in socket_scm_helper

 monitor.c                              |    1 +
 tests/qemu-iotests/socket_scm_helper.c |    2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

-- 
1.7.10.4

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

[Stratos Psomadakis]

Commit cdaa86a54 ("Add G_IO_HUP handler for socket chardev") exposed a
bug in the way the HMP monitor handles its input.  When a client closes
the connection to the monitor, tcp_chr_read() will catch the HUP
'signal' 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 can be left
in an inconsistent state (i.e. a half-finished command). Thus, without
calling readline_restart() on mon->rs upon CHR_EVENT_CLOSED, future HMP
commands will fail.

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

[Qemu-devel] [PATCH 2/2] iotests: Send the correct fd in socket_scm_helper

[Stratos Psomadakis]

Make sure to pass the correct fd via SCM_RIGHTS in socket_scm_helper.c
(i.e. fd_to_send, not socket-fd).

Signed-off-by: Stratos Psomadakis <psomas@grnet.gr>
Signed-off-by: Dimitris Aragiorgis <dimara@grnet.gr>
---
 tests/qemu-iotests/socket_scm_helper.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/socket_scm_helper.c b/tests/qemu-iotests/socket_scm_helper.c
index 0e2b285..8195983 100644
--- a/tests/qemu-iotests/socket_scm_helper.c
+++ b/tests/qemu-iotests/socket_scm_helper.c
@@ -52,7 +52,7 @@ static int send_fd(int fd, int fd_to_send)
     cmsg->cmsg_len = CMSG_LEN(sizeof(int));
     cmsg->cmsg_level = SOL_SOCKET;
     cmsg->cmsg_type = SCM_RIGHTS;
-    memcpy(CMSG_DATA(cmsg), &fd, sizeof(int));
+    memcpy(CMSG_DATA(cmsg), &fd_to_send, sizeof(int));
 
     do {
         ret = sendmsg(fd, &msg, 0);
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 0/2] Monitor-related fixes

[Markus Armbruster]

You neglected to cc maintainers.

Stratos Psomadakis <psomas@grnet.gr> writes:

> Hi,
>
> the first patch fixes an issue with HMP monitors, which was exposed
> with v2.1.0 (commits cdaa86a and 812c10).

Copying Luiz.

> The second one fixes a typo in a helper C program used in
> qemu-iotests.

Copying Kevin and Stefan.

> We think that they should be cherry-picked for the next stable release.

Copying qemu-stable@nongnu.org.

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

[Markus Armbruster]

Stratos Psomadakis <psomas@grnet.gr> writes:

> Commit cdaa86a54 ("Add G_IO_HUP handler for socket chardev") exposed a
> bug in the way the HMP monitor handles its input.  When a client closes
> the connection to the monitor, tcp_chr_read() will catch the HUP
> 'signal' and call tcp_chr_disconnect() to close the server-side
> connection too.

Your wording suggests SIGUP, but that's misleading.  Suggest
"tcp_chr_read() will detect the G_IO_HUP condition, and call".

>                 Due to the fact that monitor reads 1 byte at a time (for
> each tcp_chr_read()), the monitor readline state / buffers can be left
> in an inconsistent state (i.e. a half-finished command).

The state is not really inconsistent, there's just junk left in
rs->cmd_buf[].

>                                                          Thus, without
> calling readline_restart() on mon->rs upon CHR_EVENT_CLOSED, future HMP
> commands will fail.

To make sure I understand you correctly: when you connect again, any
leftover junk is prepended to your input, which messes up your first
command.  Correct?

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

Patch looks good to me.

Re: [Qemu-devel] [PATCH 2/2] iotests: Send the correct fd in socket_scm_helper

[Markus Armbruster]

Stratos Psomadakis <psomas@grnet.gr> writes:

> Make sure to pass the correct fd via SCM_RIGHTS in socket_scm_helper.c
> (i.e. fd_to_send, not socket-fd).
>
> Signed-off-by: Stratos Psomadakis <psomas@grnet.gr>
> Signed-off-by: Dimitris Aragiorgis <dimara@grnet.gr>
> ---
>  tests/qemu-iotests/socket_scm_helper.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/qemu-iotests/socket_scm_helper.c b/tests/qemu-iotests/socket_scm_helper.c
> index 0e2b285..8195983 100644
> --- a/tests/qemu-iotests/socket_scm_helper.c
> +++ b/tests/qemu-iotests/socket_scm_helper.c
> @@ -52,7 +52,7 @@ static int send_fd(int fd, int fd_to_send)
>      cmsg->cmsg_len = CMSG_LEN(sizeof(int));
>      cmsg->cmsg_level = SOL_SOCKET;
>      cmsg->cmsg_type = SCM_RIGHTS;
> -    memcpy(CMSG_DATA(cmsg), &fd, sizeof(int));
> +    memcpy(CMSG_DATA(cmsg), &fd_to_send, sizeof(int));
>  
>      do {
>          ret = sendmsg(fd, &msg, 0);

Ouch.  Do you have an idea what's broken without this fix?

Re: [Qemu-devel] [PATCH 2/2] iotests: Send the correct fd in socket_scm_helper

[Kevin Wolf]

Am 12.09.2014 um 09:04 hat Markus Armbruster geschrieben:
> Stratos Psomadakis <psomas@grnet.gr> writes:
> 
> > Make sure to pass the correct fd via SCM_RIGHTS in socket_scm_helper.c
> > (i.e. fd_to_send, not socket-fd).
> >
> > Signed-off-by: Stratos Psomadakis <psomas@grnet.gr>
> > Signed-off-by: Dimitris Aragiorgis <dimara@grnet.gr>

Thanks, applied to the block branch.

(Also thanks to Markus for copying me, would have missed the patch
otherwise.)

> >  tests/qemu-iotests/socket_scm_helper.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/tests/qemu-iotests/socket_scm_helper.c b/tests/qemu-iotests/socket_scm_helper.c
> > index 0e2b285..8195983 100644
> > --- a/tests/qemu-iotests/socket_scm_helper.c
> > +++ b/tests/qemu-iotests/socket_scm_helper.c
> > @@ -52,7 +52,7 @@ static int send_fd(int fd, int fd_to_send)
> >      cmsg->cmsg_len = CMSG_LEN(sizeof(int));
> >      cmsg->cmsg_level = SOL_SOCKET;
> >      cmsg->cmsg_type = SCM_RIGHTS;
> > -    memcpy(CMSG_DATA(cmsg), &fd, sizeof(int));
> > +    memcpy(CMSG_DATA(cmsg), &fd_to_send, sizeof(int));
> >  
> >      do {
> >          ret = sendmsg(fd, &msg, 0);
> 
> Ouch.  Do you have an idea what's broken without this fix?

As far as I can tell, nothing. Test case 045 will send a different file
descriptor than it intended to, but the file descriptors aren't used
other than checking whether qemu correctly reports their existence, so
it doesn't matter.

I'm not adding qemu-stable therefore. Please correct me if I'm missing
something.

Kevin

Re: [Qemu-devel] [PATCH 2/2] iotests: Send the correct fd in socket_scm_helper

[Stratos Psomadakis]

[-- Attachment #1: Type: text/plain, Size: 1790 bytes --]

On 12/09/2014 11:31 πμ, Kevin Wolf wrote:
> Am 12.09.2014 um 09:04 hat Markus Armbruster geschrieben:
>> Stratos Psomadakis <psomas@grnet.gr> writes:
>>
>>> Make sure to pass the correct fd via SCM_RIGHTS in socket_scm_helper.c
>>> (i.e. fd_to_send, not socket-fd).
>>>
>>> Signed-off-by: Stratos Psomadakis <psomas@grnet.gr>
>>> Signed-off-by: Dimitris Aragiorgis <dimara@grnet.gr>
> Thanks, applied to the block branch.
>
> (Also thanks to Markus for copying me, would have missed the patch
> otherwise.)
>
>>>  tests/qemu-iotests/socket_scm_helper.c |    2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/tests/qemu-iotests/socket_scm_helper.c b/tests/qemu-iotests/socket_scm_helper.c
>>> index 0e2b285..8195983 100644
>>> --- a/tests/qemu-iotests/socket_scm_helper.c
>>> +++ b/tests/qemu-iotests/socket_scm_helper.c
>>> @@ -52,7 +52,7 @@ static int send_fd(int fd, int fd_to_send)
>>>      cmsg->cmsg_len = CMSG_LEN(sizeof(int));
>>>      cmsg->cmsg_level = SOL_SOCKET;
>>>      cmsg->cmsg_type = SCM_RIGHTS;
>>> -    memcpy(CMSG_DATA(cmsg), &fd, sizeof(int));
>>> +    memcpy(CMSG_DATA(cmsg), &fd_to_send, sizeof(int));
>>>  
>>>      do {
>>>          ret = sendmsg(fd, &msg, 0);
>> Ouch.  Do you have an idea what's broken without this fix?
> As far as I can tell, nothing. Test case 045 will send a different file
> descriptor than it intended to, but the file descriptors aren't used
> other than checking whether qemu correctly reports their existence, so
> it doesn't matter.
>
> I'm not adding qemu-stable therefore. Please correct me if I'm missing
> something.

Right. I mentioned qemu-stable mainly for the first patch.

Thanks,
Stratos

>
> Kevin


-- 
Stratos Psomadakis
<psomas@grnet.gr>



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Stratos Psomadakis]

[-- Attachment #1: Type: text/plain, Size: 1933 bytes --]

On 12/09/2014 09:58 πμ, Markus Armbruster wrote:
> Stratos Psomadakis <psomas@grnet.gr> writes:
>
>> Commit cdaa86a54 ("Add G_IO_HUP handler for socket chardev") exposed a
>> bug in the way the HMP monitor handles its input.  When a client closes
>> the connection to the monitor, tcp_chr_read() will catch the HUP
>> 'signal' and call tcp_chr_disconnect() to close the server-side
>> connection too.
> Your wording suggests SIGUP, but that's misleading.  Suggest
> "tcp_chr_read() will detect the G_IO_HUP condition, and call".

ack

>
>>                 Due to the fact that monitor reads 1 byte at a time (for
>> each tcp_chr_read()), the monitor readline state / buffers can be left
>> in an inconsistent state (i.e. a half-finished command).
> The state is not really inconsistent, there's just junk left in
> rs->cmd_buf[].

ack


>>                                                          Thus, without
>> calling readline_restart() on mon->rs upon CHR_EVENT_CLOSED, future HMP
>> commands will fail.
> To make sure I understand you correctly: when you connect again, any
> leftover junk is prepended to your input, which messes up your first
> command.  Correct?

Yeap.

>> 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;
> Patch looks good to me.

ok, I'll edit the commit msg and resend the patch.

Thanks,
Stratos

>


-- 
Stratos Psomadakis
<psomas@grnet.gr>



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Stratos Psomadakis]

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.

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

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

[Luiz Capitulino]

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? Are you using the mux feature? We also reset it
in CHR_EVENT_OPENED if the mux feature is not used, why isn't that
good enough?

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

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

[Stratos Psomadakis]

[-- Attachment #1: Type: text/plain, Size: 2277 bytes --]

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).

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;


-- 
Stratos Psomadakis
<psomas@grnet.gr>



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

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

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

[Stratos Psomadakis]

[-- Attachment #1: Type: text/plain, Size: 3251 bytes --]

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.

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


-- 
Stratos Psomadakis
<psomas@grnet.gr>



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Luiz Capitulino]

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

[Qemu-devel] [PATCH resend v2 1/2] monitor: Reset HMP mon->rs in CHR_EVENT_OPEN

[Stratos Psomadakis]

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 in
CHR_EVENT_OPEN, future HMP commands will fail.

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..fb266bc 100644
--- a/monitor.c
+++ b/monitor.c
@@ -5245,6 +5245,7 @@ static void monitor_event(void *opaque, int event)
         monitor_printf(mon, "QEMU %s monitor - type 'help' for more "
                        "information\n", QEMU_VERSION);
         if (!mon->mux_out) {
+            readline_restart(mon->rs);
             readline_show_prompt(mon->rs);
         }
         mon->reset_seen = 1;
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH resend v2 1/2] monitor: Reset HMP mon->rs in CHR_EVENT_OPEN

[Luiz Capitulino]

On Mon, 15 Sep 2014 15:34:57 +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 in
> CHR_EVENT_OPEN, future HMP commands will fail.
> 
> Signed-off-by: Stratos Psomadakis <psomas@grnet.gr>
> Signed-off-by: Dimitris Aragiorgis <dimara@grnet.gr>

Applied to the qmp branch, thanks.

> ---
>  monitor.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/monitor.c b/monitor.c
> index 34cee74..fb266bc 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -5245,6 +5245,7 @@ static void monitor_event(void *opaque, int event)
>          monitor_printf(mon, "QEMU %s monitor - type 'help' for more "
>                         "information\n", QEMU_VERSION);
>          if (!mon->mux_out) {
> +            readline_restart(mon->rs);
>              readline_show_prompt(mon->rs);
>          }
>          mon->reset_seen = 1;