Re: [Qemu-devel] [PATCH] vl: don't use RUN_STATE_PRECONFIG unless --preconfig is given

[Michal Privoznik <mprivozn@redhat.com>]

On 06/04/2018 12:35 PM, Daniel P. Berrangé wrote:
> On Mon, Jun 04, 2018 at 12:33:04PM +0200, Max Reitz wrote:
>> On 2018-06-04 12:27, Daniel P. Berrangé wrote:
>>> The RUN_STATE_PRECONFIG state is not supposed to be reachable unless the
>>> --preconfig argument is given to QEMU, but when it was introduced in:
>>>
>>>   commit 047f7038f586d2150f16c6d9ba9cfd0479f0f6ac
>>>   Author: Igor Mammedov <imammedo@redhat.com>
>>>   Date:   Fri May 11 19:24:43 2018 +0200
>>>
>>>     cli: add --preconfig option
>>>
>>> The global 'current_run_state' variable was changed to have an initial
>>> value of RUN_STATE_PRECONFIG regardless of whether --preconfig is given.
>>>
>>> It then relies on the main loop to toggle it back to RUN_STATE_PRELAUNCH
>>> when --preconfig is not given. This is racy because it means that there
>>> is a window where QEMU is in RUN_STATE_PRECONFIG despite --preconfig not
>>> being given. This can be seen with the failure:
>>>
>>>   $ echo | x86_64-softmmu/qemu-system-x86_64 -monitor stdio
>>>   QEMU 2.12.50 monitor - type 'help' for more information
>>>   (qemu)
>>>   HMP not available in preconfig state, use QMP instead
>>>
>>> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>>> ---
>>>  vl.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> This indeed fixes the issue that the preconfig state is reachable
>> without --preconfig, but it still keeps the main loop being invoked
>> twice (which means that e.g. HMP will process a single character before
>> the main loop is actually really invoked:
>>
>> $ echo quit | x86_64-softmmu/qemu-system-x86_64 \
>>     -drive file=/dev/null,if=ide,readonly=on -monitor stdio
>> QEMU 2.12.50 monitor - type 'help' for more information
>> (qemu) qqemu-system-x86_64: Initialization of device ide-hd failed:
>> Block node is read-only
>>
>> (Note the "q" before "qemu-system-x86_64"))
>>
>> (Naively,) I agree with Michal that the main loop should only be invoked
>> twice if --preconfig has been given, which is implemented by his patch:
>>
>> http://lists.nongnu.org/archive/html/qemu-devel/2018-06/msg00367.html
> 
> I think we probably need a combination of both patches for maximum safety.

Yes, looks like that. Except when your patch is merged then:

+        runstate_set(RUN_STATE_PRELAUNCH);

from my patch becomes unnecessary. So I'll wait for you to merge your
patch and then I can resend v2 of mine.

Michal