Message ID | 20210511163151.45167-1-kwolf@redhat.com |
---|---|
State | New |
Headers | show |
Series | hmp: Fix loadvm to resume the VM on success instead of failure | expand |
On Tue, May 11, 2021 at 06:31:51PM +0200, Kevin Wolf wrote: > Commit f61fe11aa6f broke hmp_loadvm() by adding an incorrect negation > when converting from 0/-errno return values to a bool value. The result > is that loadvm resumes the VM now if it failed and keeps it stopped if > it failed. Fix it to restore the old behaviour and do it the other way > around. > > Fixes: f61fe11aa6f7f8f0ffe4ddaa56a8108f3ab57854 > Cc: qemu-stable@nongnu.org > Reported-by: Yanhui Ma <yama@redhat.com> > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > monitor/hmp-cmds.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c > index 0ad5b77477..cc15d9b6ee 100644 > --- a/monitor/hmp-cmds.c > +++ b/monitor/hmp-cmds.c > @@ -1133,7 +1133,7 @@ void hmp_loadvm(Monitor *mon, const QDict *qdict) > > vm_stop(RUN_STATE_RESTORE_VM); > > - if (!load_snapshot(name, NULL, false, NULL, &err) && saved_vm_running) { > + if (load_snapshot(name, NULL, false, NULL, &err) && saved_vm_running) { > vm_start(); > } > hmp_handle_error(mon, err); Paolo had sent a different fix here: https://lists.gnu.org/archive/html/qemu-devel/2021-05/msg01093.html As with my feedback there, I think we should be adding test coverage when fixing this. How about this: diff --git a/tests/qemu-iotests/068 b/tests/qemu-iotests/068 index 54e49c8ffa..137c5d0577 100755 --- a/tests/qemu-iotests/068 +++ b/tests/qemu-iotests/068 @@ -77,7 +77,7 @@ for extra_args in \ # Give qemu some time to boot before saving the VM state { sleep 1; printf "savevm 0\nquit\n"; } | _qemu $extra_args # Now try to continue from that VM state (this should just work) - { sleep 1; printf "loadvm 0\nloadvm 0\nquit\n"; } | _qemu $extra_args -S + { sleep 1; printf "info status\nloadvm 0\ninfo status\ncont\ninfo status\nloadvm 0\ninfo status\nquit\n"; } | _qemu $extra_args -S done # success, all done diff --git a/tests/qemu-iotests/068.out b/tests/qemu-iotests/068.out index f07a938a38..75c0a5df5f 100644 --- a/tests/qemu-iotests/068.out +++ b/tests/qemu-iotests/068.out @@ -7,8 +7,17 @@ QEMU X.Y.Z monitor - type 'help' for more information (qemu) savevm 0 (qemu) quit QEMU X.Y.Z monitor - type 'help' for more information +(qemu) info status +VM status: paused (prelaunch) (qemu) loadvm 0 +(qemu) info status +VM status: paused (prelaunch) +(qemu) cont +(qemu) info status +VM status: running (qemu) loadvm 0 +(qemu) info status +VM status: running (qemu) quit === Saving and reloading a VM state to/from a qcow2 image (-object iothread,id=iothread0 -set device.hba0.iothread=iothread0) === @@ -18,7 +27,16 @@ QEMU X.Y.Z monitor - type 'help' for more information (qemu) savevm 0 (qemu) quit QEMU X.Y.Z monitor - type 'help' for more information +(qemu) info status +VM status: paused (prelaunch) (qemu) loadvm 0 +(qemu) info status +VM status: paused (prelaunch) +(qemu) cont +(qemu) info status +VM status: running (qemu) loadvm 0 +(qemu) info status +VM status: running (qemu) quit *** done Regards, Daniel
Am 11.05.2021 um 18:49 hat Daniel P. Berrangé geschrieben: > On Tue, May 11, 2021 at 06:31:51PM +0200, Kevin Wolf wrote: > > Commit f61fe11aa6f broke hmp_loadvm() by adding an incorrect negation > > when converting from 0/-errno return values to a bool value. The result > > is that loadvm resumes the VM now if it failed and keeps it stopped if > > it failed. Fix it to restore the old behaviour and do it the other way > > around. > > > > Fixes: f61fe11aa6f7f8f0ffe4ddaa56a8108f3ab57854 > > Cc: qemu-stable@nongnu.org > > Reported-by: Yanhui Ma <yama@redhat.com> > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > --- > > monitor/hmp-cmds.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c > > index 0ad5b77477..cc15d9b6ee 100644 > > --- a/monitor/hmp-cmds.c > > +++ b/monitor/hmp-cmds.c > > @@ -1133,7 +1133,7 @@ void hmp_loadvm(Monitor *mon, const QDict *qdict) > > > > vm_stop(RUN_STATE_RESTORE_VM); > > > > - if (!load_snapshot(name, NULL, false, NULL, &err) && saved_vm_running) { > > + if (load_snapshot(name, NULL, false, NULL, &err) && saved_vm_running) { > > vm_start(); > > } > > hmp_handle_error(mon, err); > > Paolo had sent a different fix here: > > https://lists.gnu.org/archive/html/qemu-devel/2021-05/msg01093.html Hm... I missed that patch, but doesn't it just generalise the buggy HMP code instead of fixing it? That is, we still resume the VM on failure rather than on success? > As with my feedback there, I think we should be adding test coverage > when fixing this. How about this: > [...] Yes, this looks good. Kevin
On Tue, May 11, 2021 at 06:58:01PM +0200, Kevin Wolf wrote: > Am 11.05.2021 um 18:49 hat Daniel P. Berrangé geschrieben: > > On Tue, May 11, 2021 at 06:31:51PM +0200, Kevin Wolf wrote: > > > Commit f61fe11aa6f broke hmp_loadvm() by adding an incorrect negation > > > when converting from 0/-errno return values to a bool value. The result > > > is that loadvm resumes the VM now if it failed and keeps it stopped if > > > it failed. Fix it to restore the old behaviour and do it the other way > > > around. > > > > > > Fixes: f61fe11aa6f7f8f0ffe4ddaa56a8108f3ab57854 > > > Cc: qemu-stable@nongnu.org > > > Reported-by: Yanhui Ma <yama@redhat.com> > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > > --- > > > monitor/hmp-cmds.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c > > > index 0ad5b77477..cc15d9b6ee 100644 > > > --- a/monitor/hmp-cmds.c > > > +++ b/monitor/hmp-cmds.c > > > @@ -1133,7 +1133,7 @@ void hmp_loadvm(Monitor *mon, const QDict *qdict) > > > > > > vm_stop(RUN_STATE_RESTORE_VM); > > > > > > - if (!load_snapshot(name, NULL, false, NULL, &err) && saved_vm_running) { > > > + if (load_snapshot(name, NULL, false, NULL, &err) && saved_vm_running) { > > > vm_start(); > > > } > > > hmp_handle_error(mon, err); > > > > Paolo had sent a different fix here: > > > > https://lists.gnu.org/archive/html/qemu-devel/2021-05/msg01093.html > > Hm... I missed that patch, but doesn't it just generalise the buggy HMP > code instead of fixing it? That is, we still resume the VM on failure > rather than on success? Yes, if I use my iotest patch on Paolo's patch it shows that it is still broken. Regards, Daniel
* Kevin Wolf (kwolf@redhat.com) wrote: > Commit f61fe11aa6f broke hmp_loadvm() by adding an incorrect negation > when converting from 0/-errno return values to a bool value. The result > is that loadvm resumes the VM now if it failed and keeps it stopped if > it failed. Fix it to restore the old behaviour and do it the other way > around. I think your wording is wrong there - you have two 'if it failed' > Fixes: f61fe11aa6f7f8f0ffe4ddaa56a8108f3ab57854 > Cc: qemu-stable@nongnu.org > Reported-by: Yanhui Ma <yama@redhat.com> > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > monitor/hmp-cmds.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c > index 0ad5b77477..cc15d9b6ee 100644 > --- a/monitor/hmp-cmds.c > +++ b/monitor/hmp-cmds.c > @@ -1133,7 +1133,7 @@ void hmp_loadvm(Monitor *mon, const QDict *qdict) > > vm_stop(RUN_STATE_RESTORE_VM); > > - if (!load_snapshot(name, NULL, false, NULL, &err) && saved_vm_running) { > + if (load_snapshot(name, NULL, false, NULL, &err) && saved_vm_running) { > vm_start(); So if I'm reading htat right, that gets you it restarting it if load_snapshot returns true, and it returns true if the load_snapshot worked; i.e. if we were running and we succesfully load a snasphot then we carry on running. Other than the commit message, it makes sense tome: Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > } > hmp_handle_error(mon, err); > -- > 2.30.2 >
* Kevin Wolf (kwolf@redhat.com) wrote: > Commit f61fe11aa6f broke hmp_loadvm() by adding an incorrect negation > when converting from 0/-errno return values to a bool value. The result > is that loadvm resumes the VM now if it failed and keeps it stopped if > it failed. Fix it to restore the old behaviour and do it the other way > around. > > Fixes: f61fe11aa6f7f8f0ffe4ddaa56a8108f3ab57854 > Cc: qemu-stable@nongnu.org > Reported-by: Yanhui Ma <yama@redhat.com> > Signed-off-by: Kevin Wolf <kwolf@redhat.com> queued > --- > monitor/hmp-cmds.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c > index 0ad5b77477..cc15d9b6ee 100644 > --- a/monitor/hmp-cmds.c > +++ b/monitor/hmp-cmds.c > @@ -1133,7 +1133,7 @@ void hmp_loadvm(Monitor *mon, const QDict *qdict) > > vm_stop(RUN_STATE_RESTORE_VM); > > - if (!load_snapshot(name, NULL, false, NULL, &err) && saved_vm_running) { > + if (load_snapshot(name, NULL, false, NULL, &err) && saved_vm_running) { > vm_start(); > } > hmp_handle_error(mon, err); > -- > 2.30.2 >
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c index 0ad5b77477..cc15d9b6ee 100644 --- a/monitor/hmp-cmds.c +++ b/monitor/hmp-cmds.c @@ -1133,7 +1133,7 @@ void hmp_loadvm(Monitor *mon, const QDict *qdict) vm_stop(RUN_STATE_RESTORE_VM); - if (!load_snapshot(name, NULL, false, NULL, &err) && saved_vm_running) { + if (load_snapshot(name, NULL, false, NULL, &err) && saved_vm_running) { vm_start(); } hmp_handle_error(mon, err);
Commit f61fe11aa6f broke hmp_loadvm() by adding an incorrect negation when converting from 0/-errno return values to a bool value. The result is that loadvm resumes the VM now if it failed and keeps it stopped if it failed. Fix it to restore the old behaviour and do it the other way around. Fixes: f61fe11aa6f7f8f0ffe4ddaa56a8108f3ab57854 Cc: qemu-stable@nongnu.org Reported-by: Yanhui Ma <yama@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- monitor/hmp-cmds.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)