[PATCH] hmp: Fix loadvm to resume the VM on success instead of failure

All of lore.kernel.org
 help / color / mirror / Atom feed

* [PATCH] hmp: Fix loadvm to resume the VM on success instead of failure
@ 2021-05-11 16:31 Kevin Wolf
  2021-05-11 16:49 ` Daniel P. Berrangé
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Kevin Wolf @ 2021-05-11 16:31 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, berrange, qemu-devel, qemu-stable, dgilbert, yama

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);
-- 
2.30.2



^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] hmp: Fix loadvm to resume the VM on success instead of failure
  2021-05-11 16:31 [PATCH] hmp: Fix loadvm to resume the VM on success instead of failure Kevin Wolf
@ 2021-05-11 16:49 ` Daniel P. Berrangé
  2021-05-11 16:58   ` Kevin Wolf
  2021-05-12 19:16 ` Dr. David Alan Gilbert
  2021-05-25 17:43 ` Dr. David Alan Gilbert
  2 siblings, 1 reply; 6+ messages in thread
From: Daniel P. Berrangé @ 2021-05-11 16:49 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: qemu-stable, yama, dgilbert, qemu-block, qemu-devel

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
-- 
|: 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 :|



^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] hmp: Fix loadvm to resume the VM on success instead of failure
  2021-05-11 16:49 ` Daniel P. Berrangé
@ 2021-05-11 16:58   ` Kevin Wolf
  2021-05-11 17:05     ` Daniel P. Berrangé
  0 siblings, 1 reply; 6+ messages in thread
From: Kevin Wolf @ 2021-05-11 16:58 UTC (permalink / raw)
  To: Daniel P. Berrangé
  Cc: qemu-stable, yama, dgilbert, qemu-block, qemu-devel

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



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] hmp: Fix loadvm to resume the VM on success instead of failure
  2021-05-11 16:58   ` Kevin Wolf
@ 2021-05-11 17:05     ` Daniel P. Berrangé
  0 siblings, 0 replies; 6+ messages in thread
From: Daniel P. Berrangé @ 2021-05-11 17:05 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: qemu-stable, yama, dgilbert, qemu-block, qemu-devel

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
-- 
|: 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 :|



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] hmp: Fix loadvm to resume the VM on success instead of failure
  2021-05-11 16:31 [PATCH] hmp: Fix loadvm to resume the VM on success instead of failure Kevin Wolf
  2021-05-11 16:49 ` Daniel P. Berrangé
@ 2021-05-12 19:16 ` Dr. David Alan Gilbert
  2021-05-25 17:43 ` Dr. David Alan Gilbert
  2 siblings, 0 replies; 6+ messages in thread
From: Dr. David Alan Gilbert @ 2021-05-12 19:16 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: qemu-stable, berrange, yama, qemu-block, qemu-devel

* 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
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] hmp: Fix loadvm to resume the VM on success instead of failure
  2021-05-11 16:31 [PATCH] hmp: Fix loadvm to resume the VM on success instead of failure Kevin Wolf
  2021-05-11 16:49 ` Daniel P. Berrangé
  2021-05-12 19:16 ` Dr. David Alan Gilbert
@ 2021-05-25 17:43 ` Dr. David Alan Gilbert
  2 siblings, 0 replies; 6+ messages in thread
From: Dr. David Alan Gilbert @ 2021-05-25 17:43 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: qemu-stable, berrange, yama, qemu-block, qemu-devel

* 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
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK



^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-05-25 17:45 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-11 16:31 [PATCH] hmp: Fix loadvm to resume the VM on success instead of failure Kevin Wolf
2021-05-11 16:49 ` Daniel P. Berrangé
2021-05-11 16:58   ` Kevin Wolf
2021-05-11 17:05     ` Daniel P. Berrangé
2021-05-12 19:16 ` Dr. David Alan Gilbert
2021-05-25 17:43 ` Dr. David Alan Gilbert

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.