[Qemu-devel] moving CHR_EVENT_OPEN out of BH broke echo for muxed mon

[Qemu-devel] moving CHR_EVENT_OPEN out of BH broke echo for muxed mon

[Michael Tokarev]

Before

commit bd5c51ee6c4f1c79cae5ad2516d711a27b4ea8ec
Author: Michael Roth <mdroth@linux.vnet.ibm.com>
Date:   Fri Jun 7 15:19:53 2013 -0500

    qemu-char: don't issue CHR_EVENT_OPEN in a BH

we had no echo by default with -nographic, and it gave
the prompt when switching to monitor:

  $ qemu-system-x86_64 -nographic
  _

(nothing is on the screen except the command line)

  (Ctrl+A, c)
  QEMU 1.5.0 monitor - type 'help' for more information
  (qemu) _

After this patch, we now have:

  $ qemu-system-x86_64 -nographic
  QEMU 1.5.0 monitor - type 'help' for more information
  (qemu)
  _

(note the cursor position on the _next_ line after the prompt),
and the system is actually in "serial port" mode, not monitor
mode (you still need to hit Ctrl+A,c to switch to monitor).

Thanks,

/mjt

Re: [Qemu-devel] moving CHR_EVENT_OPEN out of BH broke echo for muxed mon

[Michael Roth]

On Wed, Jul 3, 2013 at 1:03 AM, Michael Tokarev <mjt@tls.msk.ru> wrote:
> Before
>
> commit bd5c51ee6c4f1c79cae5ad2516d711a27b4ea8ec
> Author: Michael Roth <mdroth@linux.vnet.ibm.com>
> Date:   Fri Jun 7 15:19:53 2013 -0500
>
>     qemu-char: don't issue CHR_EVENT_OPEN in a BH
>
> we had no echo by default with -nographic, and it gave
> the prompt when switching to monitor:
>
>   $ qemu-system-x86_64 -nographic
>   _
>
> (nothing is on the screen except the command line)
>
>   (Ctrl+A, c)
>   QEMU 1.5.0 monitor - type 'help' for more information
>   (qemu) _
>
> After this patch, we now have:
>
>   $ qemu-system-x86_64 -nographic
>   QEMU 1.5.0 monitor - type 'help' for more information
>   (qemu)
>   _
>
> (note the cursor position on the _next_ line after the prompt),
> and the system is actually in "serial port" mode, not monitor
> mode (you still need to hit Ctrl+A,c to switch to monitor).

After a bit of unwinding I think I know what's going on.

When we attach a new front-end to a mux (via qemu_chr_add_handlers), we call the
mux_chr_update_read_handler() to take those new handlers and stick them in an
array of FEs.

We then switch over to the new FE by issuing CHR_EVENT_MUX_OUT to the
previously active FE (if there was one), updating our focus idx to be the
current FE count, and issuing CHR_EVENT_MUX_IN.

Then, finally, toward the end of qemu_chr_add_handlers(), we issue a
CHR_EVENT_OPENED if the backend is already open (true in the case of stdio).

Then we repeat this for subsequent FEs, which in the normal case results in
the monitor FE getting a CHR_EVENT_MUX_OUT shortly after.

- CHR_EVENT_MUX_IN prints the prompt if EVENT_OPENED has already been sent
- CHR_EVENT_OPENED causes the monitor to output the banner and initial prompt
- CHR_EVENT_MUX_OUT causes the monitor to print a newline (to delimit any
subsequent output from another FE.

Previously, due to CHR_EVENT_OPENED getting sent in a BH, the monitor FE
would've already gotten the MUX_OUT event, so the banner and initial prompt
would be buffered, and not sent till we switched back to the monitor. Now,
we sent CHR_EVENT_OPENED before the initial MUX_OUT, so the banner actually
gets display before we switch over to the next FE.

The start-up state is actually exactly what you'd see if you cycled once
through all the FEs in prior versions of QEMU, resulting in all this
buffered up output getting flushed.

Fix should be simple: defer CHR_EVENT_OPENED until after MUX_OUT to retain
the previous behavior. Gonna be out this week but can send a patch when I
get back.

Thanks for the catch.

>
> Thanks,
>
> /mjt
>

Re: [Qemu-devel] moving CHR_EVENT_OPEN out of BH broke echo for muxed mon

[Michael Tokarev]

Can we try to fix this for 1.6 please? :)
Or should I try to cook up a patch?

Thanks,

/mjt

04.07.2013 01:06, Michael Roth wrote:
> On Wed, Jul 3, 2013 at 1:03 AM, Michael Tokarev <mjt@tls.msk.ru> wrote:
>> Before
>>
>> commit bd5c51ee6c4f1c79cae5ad2516d711a27b4ea8ec
>> Author: Michael Roth <mdroth@linux.vnet.ibm.com>
>> Date:   Fri Jun 7 15:19:53 2013 -0500
>>
>>      qemu-char: don't issue CHR_EVENT_OPEN in a BH
>>
>> we had no echo by default with -nographic, and it gave
>> the prompt when switching to monitor:
>>
>>    $ qemu-system-x86_64 -nographic
>>    _
>>
>> (nothing is on the screen except the command line)
>>
>>    (Ctrl+A, c)
>>    QEMU 1.5.0 monitor - type 'help' for more information
>>    (qemu) _
>>
>> After this patch, we now have:
>>
>>    $ qemu-system-x86_64 -nographic
>>    QEMU 1.5.0 monitor - type 'help' for more information
>>    (qemu)
>>    _
>>
>> (note the cursor position on the _next_ line after the prompt),
>> and the system is actually in "serial port" mode, not monitor
>> mode (you still need to hit Ctrl+A,c to switch to monitor).
>
> After a bit of unwinding I think I know what's going on.
>
> When we attach a new front-end to a mux (via qemu_chr_add_handlers), we call the
> mux_chr_update_read_handler() to take those new handlers and stick them in an
> array of FEs.
>
> We then switch over to the new FE by issuing CHR_EVENT_MUX_OUT to the
> previously active FE (if there was one), updating our focus idx to be the
> current FE count, and issuing CHR_EVENT_MUX_IN.
>
> Then, finally, toward the end of qemu_chr_add_handlers(), we issue a
> CHR_EVENT_OPENED if the backend is already open (true in the case of stdio).
>
> Then we repeat this for subsequent FEs, which in the normal case results in
> the monitor FE getting a CHR_EVENT_MUX_OUT shortly after.
>
> - CHR_EVENT_MUX_IN prints the prompt if EVENT_OPENED has already been sent
> - CHR_EVENT_OPENED causes the monitor to output the banner and initial prompt
> - CHR_EVENT_MUX_OUT causes the monitor to print a newline (to delimit any
> subsequent output from another FE.
>
> Previously, due to CHR_EVENT_OPENED getting sent in a BH, the monitor FE
> would've already gotten the MUX_OUT event, so the banner and initial prompt
> would be buffered, and not sent till we switched back to the monitor. Now,
> we sent CHR_EVENT_OPENED before the initial MUX_OUT, so the banner actually
> gets display before we switch over to the next FE.
>
> The start-up state is actually exactly what you'd see if you cycled once
> through all the FEs in prior versions of QEMU, resulting in all this
> buffered up output getting flushed.
>
> Fix should be simple: defer CHR_EVENT_OPENED until after MUX_OUT to retain
> the previous behavior. Gonna be out this week but can send a patch when I
> get back.
>
> Thanks for the catch.
>
>>
>> Thanks,
>>
>> /mjt
>>
>

Re: [Qemu-devel] moving CHR_EVENT_OPEN out of BH broke echo for muxed mon

[Michael Roth]

Quoting Michael Tokarev (2013-07-29 06:26:20)
> Can we try to fix this for 1.6 please? :)
> Or should I try to cook up a patch?

Hmm, this seems to be a little less straight-forward than I thought. We can't
defer OPENED till after MUX_OUT ad infinitum since this will cause the latest
FE to be added to never get the OPENED event. And new FEs can get added all
throughout QEMU's lifecycle so there's no way to know when we're on the 'last'
FE and can thus emit the deferred OPENED event unless we hook into something
like qemu_add_machine_init_done_notifier().

Otherwise I don't see a solution other than re-introducing a bottom-half of
some sort. If it's via g_idle_timeout() we'll have the same problems as before
though, so this would involve using a QEMUBH which negates the work of being
able to drive chardevs via a generic GMainLoop.

Also, previous semantics were "on the subsequent iteration of the main loop,
send OPENED event for all new FEs that have been added", and whoever had
the focus at that time would print it's prompt/banner/etc output for OPENED
events, so a QEMUBH is the only way to maintain the semantics exactly. With
qemu_add_machine_init_done_notifier() approach, we'd see different behavior
if, for instance, 2 FEs were attached to the mux in a single iteration of
the main loop, as each would get the OPENED event immediately, as opposed
to the previous behavior where only the latest one would get it before
MUX_OUT was issued.

Personally I think the slight change in semantics is worth avoiding
re-introducing a QEMUBH, so I'll go ahead and put something together using
qemu_add_machine_init_done_notifer(), but it feels dirty so please chime in
if there's might be a better way to do this.

> 
> Thanks,
> 
> /mjt
> 
> 04.07.2013 01:06, Michael Roth wrote:
> > On Wed, Jul 3, 2013 at 1:03 AM, Michael Tokarev <mjt@tls.msk.ru> wrote:
> >> Before
> >>
> >> commit bd5c51ee6c4f1c79cae5ad2516d711a27b4ea8ec
> >> Author: Michael Roth <mdroth@linux.vnet.ibm.com>
> >> Date:   Fri Jun 7 15:19:53 2013 -0500
> >>
> >>      qemu-char: don't issue CHR_EVENT_OPEN in a BH
> >>
> >> we had no echo by default with -nographic, and it gave
> >> the prompt when switching to monitor:
> >>
> >>    $ qemu-system-x86_64 -nographic
> >>    _
> >>
> >> (nothing is on the screen except the command line)
> >>
> >>    (Ctrl+A, c)
> >>    QEMU 1.5.0 monitor - type 'help' for more information
> >>    (qemu) _
> >>
> >> After this patch, we now have:
> >>
> >>    $ qemu-system-x86_64 -nographic
> >>    QEMU 1.5.0 monitor - type 'help' for more information
> >>    (qemu)
> >>    _
> >>
> >> (note the cursor position on the _next_ line after the prompt),
> >> and the system is actually in "serial port" mode, not monitor
> >> mode (you still need to hit Ctrl+A,c to switch to monitor).
> >
> > After a bit of unwinding I think I know what's going on.
> >
> > When we attach a new front-end to a mux (via qemu_chr_add_handlers), we call the
> > mux_chr_update_read_handler() to take those new handlers and stick them in an
> > array of FEs.
> >
> > We then switch over to the new FE by issuing CHR_EVENT_MUX_OUT to the
> > previously active FE (if there was one), updating our focus idx to be the
> > current FE count, and issuing CHR_EVENT_MUX_IN.
> >
> > Then, finally, toward the end of qemu_chr_add_handlers(), we issue a
> > CHR_EVENT_OPENED if the backend is already open (true in the case of stdio).
> >
> > Then we repeat this for subsequent FEs, which in the normal case results in
> > the monitor FE getting a CHR_EVENT_MUX_OUT shortly after.
> >
> > - CHR_EVENT_MUX_IN prints the prompt if EVENT_OPENED has already been sent
> > - CHR_EVENT_OPENED causes the monitor to output the banner and initial prompt
> > - CHR_EVENT_MUX_OUT causes the monitor to print a newline (to delimit any
> > subsequent output from another FE.
> >
> > Previously, due to CHR_EVENT_OPENED getting sent in a BH, the monitor FE
> > would've already gotten the MUX_OUT event, so the banner and initial prompt
> > would be buffered, and not sent till we switched back to the monitor. Now,
> > we sent CHR_EVENT_OPENED before the initial MUX_OUT, so the banner actually
> > gets display before we switch over to the next FE.
> >
> > The start-up state is actually exactly what you'd see if you cycled once
> > through all the FEs in prior versions of QEMU, resulting in all this
> > buffered up output getting flushed.
> >
> > Fix should be simple: defer CHR_EVENT_OPENED until after MUX_OUT to retain
> > the previous behavior. Gonna be out this week but can send a patch when I
> > get back.
> >
> > Thanks for the catch.
> >
> >>
> >> Thanks,
> >>
> >> /mjt
> >>
> >