diff mbox series

vl: don't use RUN_STATE_PRECONFIG unless --preconfig is given

Message ID 20180604102752.32260-1-berrange@redhat.com
State New
Headers show
Series vl: don't use RUN_STATE_PRECONFIG unless --preconfig is given | expand

Commit Message

Daniel P. Berrangé June 4, 2018, 10:27 a.m. UTC 
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(-)

Comments

Max Reitz June 4, 2018, 10:33 a.m. UTC | #1 
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

Max
Daniel P. Berrangé June 4, 2018, 10:35 a.m. UTC | #2 
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.

Regards,
Daniel
Max Reitz June 4, 2018, 10:35 a.m. UTC | #3 
On 2018-06-04 12:35, 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.

Sounds good to me.

Max
Michal Prívozník June 4, 2018, 10:41 a.m. UTC | #4 
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
Igor Mammedov June 4, 2018, 11:58 a.m. UTC | #5 
On Mon, 4 Jun 2018 12:33:04 +0200
Max Reitz <mreitz@redhat.com> 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.
That was intentional, see 8a36283e12 commit message which has it right.
I should fix doc comment qapi/run-state.json though as it's misleading
(I've meant it reachable for user, while doc comment applies to
not only to user but to qemu internals as well).
I'll post a patch to clarify doc comment.

Idea behind PRECONFIG runstate is to start spitting PRELAUNCH
runstate (which is historically ended up as a dump for almost everything)
into smaller more manageable part.

Michal's patch should fix issue short term and
I'll look more at the issues Max found out, maybe there is more to fix.

> > 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
> 
> Max
>
Daniel P. Berrangé June 4, 2018, 12:05 p.m. UTC | #6 
On Mon, Jun 04, 2018 at 01:58:02PM +0200, Igor Mammedov wrote:
> On Mon, 4 Jun 2018 12:33:04 +0200
> Max Reitz <mreitz@redhat.com> 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.
> That was intentional, see 8a36283e12 commit message which has it right.
> I should fix doc comment qapi/run-state.json though as it's misleading
> (I've meant it reachable for user, while doc comment applies to
> not only to user but to qemu internals as well).
> I'll post a patch to clarify doc comment.
> 
> Idea behind PRECONFIG runstate is to start spitting PRELAUNCH
> runstate (which is historically ended up as a dump for almost everything)
> into smaller more manageable part.

I know it was intentional, but it is still undesirable, as it results in
need for bogus transitions to INMIGRATE, as well as the problems mentioned
by Michael and Max. IIUC this can all be solved by introducing a further
RUN_STATE_NONE to act as the initial value, avoiding the visibilty of
RUN_STATE_PRECONFIG unless actually requested. I've sent a further patch
that does this.

Regards,
Daniel
diff mbox series

Patch

diff --git a/vl.c b/vl.c
index 06031715ac..f776d65801 100644
--- a/vl.c
+++ b/vl.c
@@ -561,7 +561,7 @@  static int default_driver_check(void *opaque, QemuOpts *opts, Error **errp)
 /***********************************************************/
 /* QEMU state */
 
-static RunState current_run_state = RUN_STATE_PRECONFIG;
+static RunState current_run_state = RUN_STATE_PRELAUNCH;
 
 /* We use RUN_STATE__MAX but any invalid value will do */
 static RunState vmstop_requested = RUN_STATE__MAX;
@@ -3572,6 +3572,7 @@  int main(int argc, char **argv, char **envp)
                 }
                 break;
             case QEMU_OPTION_preconfig:
+                current_run_state = RUN_STATE_PRECONFIG;
                 preconfig_exit_requested = false;
                 break;
             case QEMU_OPTION_enable_kvm: