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

PATCH In review Daniel P. Berrangé 4 participants 7 messages Jun 4, 2018

Share & tools

Daniel P. Berrangé

author

2018-06-04 10:28 UTC #0 lore raw

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

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 <[email protected]>
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é <[email protected]>

---
 vl.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

vl.c +2 -1

index 06031715ac..f776d65801 100644
@@ -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:

Signature · 1 line

2.17.0

Max Reitz

2018-06-04 10:33 UTC #1 lore raw

Quoted 22 lines · Daniel P. Berrangé

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 <[email protected]>
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

Quoted 4 lines

Signed-off-by Daniel P. Berrangé <[email protected]>
---
 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é

author

2018-06-04 10:35 UTC #2 lore raw

Quoted 39 lines · Max Reitz

Quoted 22 lines · Daniel P. Berrangé
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 <[email protected]>
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
Quoted 4 lines
Signed-off-by Daniel P. Berrangé <[email protected]>
---
 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

Quoted 6 lines

(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

Signature · 3 lines

|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Max Reitz

2018-06-04 10:35 UTC #3 lore raw

Quoted 40 lines · Daniel P. Berrangé

Quoted 39 lines · Max Reitz
Quoted 22 lines · Daniel P. Berrangé
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 <[email protected]>
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
Quoted 4 lines
Signed-off-by Daniel P. Berrangé <[email protected]>
---
 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

Quoted 8 lines

Quoted 6 lines
(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 Privoznik

2018-06-04 10:41 UTC #4 lore raw

In reply to Daniel P. Berrangé

Quoted 40 lines · Daniel P. Berrangé

Quoted 39 lines · Max Reitz
Quoted 22 lines · Daniel P. Berrangé
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 <[email protected]>
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
Quoted 4 lines
Signed-off-by Daniel P. Berrangé <[email protected]>
---
 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

Quoted 8 lines

Quoted 6 lines
(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

2018-06-04 11:58 UTC #5 lore raw

In reply to Max Reitz

Quoted 4 lines · On Mon, 4 Jun 2018 12:33:04 +0200 Max Reitz <[email protected]> wrote:

Quoted 3 lines · Daniel P. Berrangé
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:

Quoted 8 lines

Quoted 8 lines
commit 047f7038f586d2150f16c6d9ba9cfd0479f0f6ac
Author: Igor Mammedov <[email protected]>
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.

Quoted 35 lines

Quoted 10 lines
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
Quoted 4 lines
Signed-off-by Daniel P. Berrangé <[email protected]>
---
 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é

author

2018-06-04 12:05 UTC #6 lore raw

Quoted 21 lines · Igor Mammedov

Quoted 4 lines · On Mon, 4 Jun 2018 12:33:04 +0200 Max Reitz <[email protected]> wrote:
Quoted 3 lines · Daniel P. Berrangé
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:
Quoted 8 lines
Quoted 8 lines
commit 047f7038f586d2150f16c6d9ba9cfd0479f0f6ac
Author: Igor Mammedov <[email protected]>
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.

Quoted 3 lines

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

Signature · 3 lines

|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|