[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

[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 <pso...@grnet.gr>
Signed-off-by: Dimitris Aragiorgis <dim...@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

[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 <pso...@grnet.gr>
Signed-off-by: Dimitris Aragiorgis <dim...@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

[Markus Armbruster]

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

[Markus Armbruster]

You neglected to cc maintainers.

Stratos Psomadakis <pso...@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-...@nongnu.org.

[Markus Armbruster]

Stratos Psomadakis <pso...@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 <pso...@grnet.gr>
> Signed-off-by: Dimitris Aragiorgis <dim...@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.

[Kevin Wolf]

Am 12.09.2014 um 09:04 hat Markus Armbruster geschrieben:

> Stratos Psomadakis <pso...@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 <pso...@grnet.gr>
> > Signed-off-by: Dimitris Aragiorgis <dim...@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

[Stratos Psomadakis]

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

Thanks,
Stratos

>
> Kevin


--
Stratos Psomadakis
<pso...@grnet.gr>

[Stratos Psomadakis]

On 12/09/2014 09:58 πμ, Markus Armbruster wrote:
> Stratos Psomadakis <pso...@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 <pso...@grnet.gr>
>> Signed-off-by: Dimitris Aragiorgis <dim...@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
<pso...@grnet.gr>

[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 <pso...@grnet.gr>
Signed-off-by: Dimitris Aragiorgis <dim...@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

[Luiz Capitulino]

On Fri, 12 Sep 2014 17:07:32 +0300
Stratos Psomadakis <pso...@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?

[Stratos Psomadakis]

On 12/09/2014 06:21 μμ, Luiz Capitulino wrote:
> On Fri, 12 Sep 2014 17:07:32 +0300
> Stratos Psomadakis <pso...@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 <pso...@grnet.gr>
>> Signed-off-by: Dimitris Aragiorgis <dim...@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
<pso...@grnet.gr>

[Luiz Capitulino]

On Fri, 12 Sep 2014 20:01:04 +0300

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?

[Stratos Psomadakis]

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 <pso...@grnet.gr>
>>>> Signed-off-by: Dimitris Aragiorgis <dim...@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
<pso...@grnet.gr>

[Luiz Capitulino]

On Sat, 13 Sep 2014 19:27:46 +0300

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

[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 <pso...@grnet.gr>
Signed-off-by: Dimitris Aragiorgis <dim...@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

[Luiz Capitulino]

On Mon, 15 Sep 2014 15:34:57 +0300
Stratos Psomadakis <pso...@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 <pso...@grnet.gr>
> Signed-off-by: Dimitris Aragiorgis <dim...@grnet.gr>

Applied to the qmp branch, thanks.