* [Qemu-devel] [PATCH 0/2] Monitor-related fixes @ 2014-09-11 15:19 Stratos Psomadakis 2014-09-11 15:19 ` [Qemu-devel] [PATCH 1/2] monitor: Reset HMP mon->rs on CHR_EVENT_CLOSED Stratos Psomadakis ` (2 more replies) 0 siblings, 3 replies; 17+ messages in thread From: Stratos Psomadakis @ 2014-09-11 15:19 UTC (permalink / raw) To: qemu-devel; +Cc: synnefo-devel 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH 1/2] monitor: Reset HMP mon->rs on CHR_EVENT_CLOSED 2014-09-11 15:19 [Qemu-devel] [PATCH 0/2] Monitor-related fixes Stratos Psomadakis @ 2014-09-11 15:19 ` Stratos Psomadakis 2014-09-12 6:58 ` Markus Armbruster 2014-09-11 15:19 ` [Qemu-devel] [PATCH 2/2] iotests: Send the correct fd in socket_scm_helper Stratos Psomadakis 2014-09-12 6:49 ` [Qemu-devel] [PATCH 0/2] Monitor-related fixes Markus Armbruster 2 siblings, 1 reply; 17+ messages in thread From: Stratos Psomadakis @ 2014-09-11 15:19 UTC (permalink / raw) To: qemu-devel; +Cc: synnefo-devel 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] monitor: Reset HMP mon->rs on CHR_EVENT_CLOSED 2014-09-11 15:19 ` [Qemu-devel] [PATCH 1/2] monitor: Reset HMP mon->rs on CHR_EVENT_CLOSED Stratos Psomadakis @ 2014-09-12 6:58 ` Markus Armbruster 2014-09-12 13:53 ` [Qemu-devel] [synnefo-devel] " Stratos Psomadakis 2014-09-12 14:07 ` [Qemu-devel] [PATCH resend " Stratos Psomadakis 0 siblings, 2 replies; 17+ messages in thread From: Markus Armbruster @ 2014-09-12 6:58 UTC (permalink / raw) To: Stratos Psomadakis; +Cc: synnefo-devel, qemu-devel 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [synnefo-devel] Re: [PATCH 1/2] monitor: Reset HMP mon->rs on CHR_EVENT_CLOSED 2014-09-12 6:58 ` Markus Armbruster @ 2014-09-12 13:53 ` Stratos Psomadakis 2014-09-12 14:07 ` [Qemu-devel] [PATCH resend " Stratos Psomadakis 1 sibling, 0 replies; 17+ messages in thread From: Stratos Psomadakis @ 2014-09-12 13:53 UTC (permalink / raw) To: Markus Armbruster; +Cc: synnefo-devel, qemu-stable, qemu-devel, lcapitulino [-- 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 --] ^ permalink raw reply [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH resend 1/2] monitor: Reset HMP mon->rs on CHR_EVENT_CLOSED 2014-09-12 6:58 ` Markus Armbruster 2014-09-12 13:53 ` [Qemu-devel] [synnefo-devel] " Stratos Psomadakis @ 2014-09-12 14:07 ` Stratos Psomadakis 2014-09-12 15:21 ` Luiz Capitulino 1 sibling, 1 reply; 17+ messages in thread From: Stratos Psomadakis @ 2014-09-12 14:07 UTC (permalink / raw) To: qemu-devel; +Cc: synnefo-devel, qemu-stable, armbru, lcapitulino 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH resend 1/2] monitor: Reset HMP mon->rs on CHR_EVENT_CLOSED 2014-09-12 14:07 ` [Qemu-devel] [PATCH resend " Stratos Psomadakis @ 2014-09-12 15:21 ` Luiz Capitulino 2014-09-12 17:01 ` [Qemu-devel] [synnefo-devel] " Stratos Psomadakis 0 siblings, 1 reply; 17+ messages in thread From: Luiz Capitulino @ 2014-09-12 15:21 UTC (permalink / raw) To: Stratos Psomadakis; +Cc: synnefo-devel, qemu-devel, armbru, qemu-stable 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; ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [synnefo-devel] Re: [PATCH resend 1/2] monitor: Reset HMP mon->rs on CHR_EVENT_CLOSED 2014-09-12 15:21 ` Luiz Capitulino @ 2014-09-12 17:01 ` Stratos Psomadakis 2014-09-12 17:19 ` Luiz Capitulino 0 siblings, 1 reply; 17+ messages in thread From: Stratos Psomadakis @ 2014-09-12 17:01 UTC (permalink / raw) To: Luiz Capitulino; +Cc: synnefo-devel, qemu-devel, armbru, qemu-stable [-- 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 --] ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [synnefo-devel] Re: [PATCH resend 1/2] monitor: Reset HMP mon->rs on CHR_EVENT_CLOSED 2014-09-12 17:01 ` [Qemu-devel] [synnefo-devel] " Stratos Psomadakis @ 2014-09-12 17:19 ` Luiz Capitulino 2014-09-13 16:27 ` Stratos Psomadakis 0 siblings, 1 reply; 17+ messages in thread From: Luiz Capitulino @ 2014-09-12 17:19 UTC (permalink / raw) To: Stratos Psomadakis; +Cc: synnefo-devel, qemu-devel, armbru, qemu-stable 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; > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [synnefo-devel] Re: [PATCH resend 1/2] monitor: Reset HMP mon->rs on CHR_EVENT_CLOSED 2014-09-12 17:19 ` Luiz Capitulino @ 2014-09-13 16:27 ` Stratos Psomadakis 2014-09-14 1:23 ` Luiz Capitulino 0 siblings, 1 reply; 17+ messages in thread From: Stratos Psomadakis @ 2014-09-13 16:27 UTC (permalink / raw) To: Luiz Capitulino; +Cc: synnefo-devel, qemu-devel, armbru, qemu-stable [-- 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 --] ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [synnefo-devel] Re: [PATCH resend 1/2] monitor: Reset HMP mon->rs on CHR_EVENT_CLOSED 2014-09-13 16:27 ` Stratos Psomadakis @ 2014-09-14 1:23 ` Luiz Capitulino 2014-09-15 12:34 ` [Qemu-devel] [PATCH resend v2 1/2] monitor: Reset HMP mon->rs in CHR_EVENT_OPEN Stratos Psomadakis 0 siblings, 1 reply; 17+ messages in thread From: Luiz Capitulino @ 2014-09-14 1:23 UTC (permalink / raw) To: Stratos Psomadakis; +Cc: synnefo-devel, qemu-devel, armbru, qemu-stable 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; > >> > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH resend v2 1/2] monitor: Reset HMP mon->rs in CHR_EVENT_OPEN 2014-09-14 1:23 ` Luiz Capitulino @ 2014-09-15 12:34 ` Stratos Psomadakis 2014-09-15 14:23 ` Luiz Capitulino 0 siblings, 1 reply; 17+ messages in thread From: Stratos Psomadakis @ 2014-09-15 12:34 UTC (permalink / raw) To: lcapitulino; +Cc: synnefo-devel, qemu-devel, armbru, qemu-stable 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH resend v2 1/2] monitor: Reset HMP mon->rs in CHR_EVENT_OPEN 2014-09-15 12:34 ` [Qemu-devel] [PATCH resend v2 1/2] monitor: Reset HMP mon->rs in CHR_EVENT_OPEN Stratos Psomadakis @ 2014-09-15 14:23 ` Luiz Capitulino 0 siblings, 0 replies; 17+ messages in thread From: Luiz Capitulino @ 2014-09-15 14:23 UTC (permalink / raw) To: Stratos Psomadakis; +Cc: synnefo-devel, qemu-devel, armbru, qemu-stable 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; ^ permalink raw reply [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH 2/2] iotests: Send the correct fd in socket_scm_helper 2014-09-11 15:19 [Qemu-devel] [PATCH 0/2] Monitor-related fixes Stratos Psomadakis 2014-09-11 15:19 ` [Qemu-devel] [PATCH 1/2] monitor: Reset HMP mon->rs on CHR_EVENT_CLOSED Stratos Psomadakis @ 2014-09-11 15:19 ` Stratos Psomadakis 2014-09-12 7:04 ` Markus Armbruster 2014-09-12 6:49 ` [Qemu-devel] [PATCH 0/2] Monitor-related fixes Markus Armbruster 2 siblings, 1 reply; 17+ messages in thread From: Stratos Psomadakis @ 2014-09-11 15:19 UTC (permalink / raw) To: qemu-devel; +Cc: synnefo-devel 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] iotests: Send the correct fd in socket_scm_helper 2014-09-11 15:19 ` [Qemu-devel] [PATCH 2/2] iotests: Send the correct fd in socket_scm_helper Stratos Psomadakis @ 2014-09-12 7:04 ` Markus Armbruster 2014-09-12 8:31 ` Kevin Wolf 0 siblings, 1 reply; 17+ messages in thread From: Markus Armbruster @ 2014-09-12 7:04 UTC (permalink / raw) To: Stratos Psomadakis; +Cc: Kevin Wolf, synnefo-devel, qemu-devel, Stefan Hajnoczi 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? ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] iotests: Send the correct fd in socket_scm_helper 2014-09-12 7:04 ` Markus Armbruster @ 2014-09-12 8:31 ` Kevin Wolf 2014-09-12 13:47 ` Stratos Psomadakis 0 siblings, 1 reply; 17+ messages in thread From: Kevin Wolf @ 2014-09-12 8:31 UTC (permalink / raw) To: Markus Armbruster Cc: synnefo-devel, Stratos Psomadakis, qemu-devel, Stefan Hajnoczi 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] iotests: Send the correct fd in socket_scm_helper 2014-09-12 8:31 ` Kevin Wolf @ 2014-09-12 13:47 ` Stratos Psomadakis 0 siblings, 0 replies; 17+ messages in thread From: Stratos Psomadakis @ 2014-09-12 13:47 UTC (permalink / raw) To: Kevin Wolf; +Cc: synnefo-devel, Markus Armbruster, Stefan Hajnoczi, qemu-devel [-- 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 --] ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 0/2] Monitor-related fixes 2014-09-11 15:19 [Qemu-devel] [PATCH 0/2] Monitor-related fixes Stratos Psomadakis 2014-09-11 15:19 ` [Qemu-devel] [PATCH 1/2] monitor: Reset HMP mon->rs on CHR_EVENT_CLOSED Stratos Psomadakis 2014-09-11 15:19 ` [Qemu-devel] [PATCH 2/2] iotests: Send the correct fd in socket_scm_helper Stratos Psomadakis @ 2014-09-12 6:49 ` Markus Armbruster 2 siblings, 0 replies; 17+ messages in thread From: Markus Armbruster @ 2014-09-12 6:49 UTC (permalink / raw) To: Stratos Psomadakis Cc: Kevin Wolf, qemu-stable, qemu-devel, synnefo-devel, Stefan Hajnoczi, Luiz Capitulino 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2014-09-15 14:24 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-09-11 15:19 [Qemu-devel] [PATCH 0/2] Monitor-related fixes Stratos Psomadakis 2014-09-11 15:19 ` [Qemu-devel] [PATCH 1/2] monitor: Reset HMP mon->rs on CHR_EVENT_CLOSED Stratos Psomadakis 2014-09-12 6:58 ` Markus Armbruster 2014-09-12 13:53 ` [Qemu-devel] [synnefo-devel] " Stratos Psomadakis 2014-09-12 14:07 ` [Qemu-devel] [PATCH resend " Stratos Psomadakis 2014-09-12 15:21 ` Luiz Capitulino 2014-09-12 17:01 ` [Qemu-devel] [synnefo-devel] " Stratos Psomadakis 2014-09-12 17:19 ` Luiz Capitulino 2014-09-13 16:27 ` Stratos Psomadakis 2014-09-14 1:23 ` Luiz Capitulino 2014-09-15 12:34 ` [Qemu-devel] [PATCH resend v2 1/2] monitor: Reset HMP mon->rs in CHR_EVENT_OPEN Stratos Psomadakis 2014-09-15 14:23 ` Luiz Capitulino 2014-09-11 15:19 ` [Qemu-devel] [PATCH 2/2] iotests: Send the correct fd in socket_scm_helper Stratos Psomadakis 2014-09-12 7:04 ` Markus Armbruster 2014-09-12 8:31 ` Kevin Wolf 2014-09-12 13:47 ` Stratos Psomadakis 2014-09-12 6:49 ` [Qemu-devel] [PATCH 0/2] Monitor-related fixes Markus Armbruster
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.