Re: [Qemu-devel] [PATCH] vl: don't use RUN_STATE_PRECONFIG unless --preconfig is given
On Mon, Jun 04, 2018 at 01:58:02PM +0200, Igor Mammedov wrote: > On Mon, 4 Jun 2018 12:33:04 +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 > > > 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 -- |: 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 :|
Re: [Qemu-devel] [PATCH] vl: don't use RUN_STATE_PRECONFIG unless --preconfig is given
On Mon, 4 Jun 2018 12:33:04 +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 > > 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é > > --- > > 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 >
Re: [Qemu-devel] [PATCH] vl: don't use RUN_STATE_PRECONFIG unless --preconfig is given
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 >>> 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é >>> --- >>> 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
Re: [Qemu-devel] [PATCH] vl: don't use RUN_STATE_PRECONFIG unless --preconfig is given
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 >>> 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é >>> --- >>> 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 signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH] vl: don't use RUN_STATE_PRECONFIG unless --preconfig is given
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 > > 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é > > --- > > 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 -- |: 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 :|
Re: [Qemu-devel] [PATCH] vl: don't use RUN_STATE_PRECONFIG unless --preconfig is given
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 > 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é > --- > 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 signature.asc Description: OpenPGP digital signature
