Re: [Qemu-devel] Re: [PATCH v2 3/3] char: emit the OPENED event only when a new char connection is opened

[Jan Kiszka]

Amit Shah wrote:
> On (Mon) Oct 26 2009 [08:40:12], Jan Kiszka wrote:
>> Amit Shah wrote:
>>> On (Sat) Oct 24 2009 [12:36:54], Jan Kiszka wrote:
>>>> Amit Shah wrote:
>>>>> The OPENED event gets sent also when qemu resets its state initially.
>>>>> The consumers of the event aren't interested in receiving this event
>>>>> on reset.
>>>> The monitor was. Now its initial prompt on activation is broken.
>>> The patch in Anthony's queue, titled
>>>
>>> 'console: call qemu_chr_reset() in text_console_init'
>> You may also want to rename qemu_chr_reset - unless there is still a
>> need for real "reset".
> 
> Yeah; there isn't a need after my patches -- I've been slowing working
> towards renaming it all.
> 
>>> fixed that.
>>>
>>> However, with the qcow2 synchronous patch, the monitor prompt doesn't
>>> come up again -- which shows there is a problem with the way the bhs
>>> work and also the initial resets.
>> Then the qcow2 patch is already in? At least applying your patch doesn't
>> change the picture.
> 
> Yeah; that patch went in just before my patches. Can you try reverting
> 
> ef845c3bf421290153154635dc18eaa677cecb43

Makes no difference either - but it also does not touch any code that
code impact the open/reset signaling.

> 
>>> I think the initial resets are a hack to work around something from my
>>> reading of it; do you have a better idea of why it's there and how it's
>>> all supposed to work?
>> From the monitor's POV, it's not a hack, it's simply the requirement to
>> receive an indication that the console was opened.
> 
> Just an indication that the monitor was opened -- agreed. But git
> history shows you added that as 'reset', so I'm wondering if maybe you
> wanted it to do something else as well (or you did it that way just
> because of the way qemu's bhs are handled?).

I didn't add the reset hook for the monitor (it was Anthony), I just
made some improvements.

> 
>>>> Does this patch fix/improve something for a different user? If not,
>>>> please let us revert it.
>>> There's another question too: is a separate 'reset' event needed in
>>> addition to an 'opened' event?
>> Not for the monitor, but I cannot speak for other users. I think it
>> would be good to check them in details before changing the reset/open
>> semantic.
> 
> As far as I could see in the git history, the 'reset' was added for the
> monitor. And the others could live with the double 'reset' events they
> were getting -- one for the reset and one when the device was opened. 

OK.

However, the problems of your approach to avoid potential double resets
on startup is that the supposed two events for the monitor (first one
from qemu_chr_open_fs, second one via qemu_chr_initial_reset) actually
coalesce into one, thus are never delivered. So whatever may happen
later on, during startup the skipping of the first reset/open event is
bogus.

Jan

signature.asc
Description: OpenPGP digital signature