[Qemu-devel] [PATCH] fix broken migration

[Qemu-devel] [PATCH] fix broken migration

[Glauber Costa]

While fixing migration with -S, commit
89befdd1a6b18215153b8976682d57b7d03d5782 broke the rest of us. Poor
glommer, with a poor family, spare him his life from this monstruosity.

Since the unconditional vm_start, not autostart was the villain, I'm putting
back autostart. Let me know if you prefer other solutions, it doesn't really matter,
doesn't really matter to me.

Any way the wind blows...

Signed-off-by: Glauber Costa <glommer@redhat.com>
CC: Paolo Bonzini <pbonzini@redhat.com>
---
 vl.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/vl.c b/vl.c
index ce213c2..015f631 100644
--- a/vl.c
+++ b/vl.c
@@ -6075,8 +6075,10 @@ int main(int argc, char **argv, char **envp)
     if (loadvm)
         do_loadvm(cur_mon, loadvm);
 
-    if (incoming)
+    if (incoming) {
+        autostart = 0;
         qemu_start_incoming_migration(incoming);
+    }
 
     if (autostart)
         vm_start();
-- 
1.6.2.2

[Qemu-devel] Re: [PATCH] fix broken migration

[Paolo Bonzini]

On 07/24/2009 10:20 PM, Glauber Costa wrote:
> While fixing migration with -S, commit
> 89befdd1a6b18215153b8976682d57b7d03d5782 broke the rest of us.

With this patch a migrated virtual machine will never be restarted.  If 
you want that, shouldn't you use -S?  But if you really wanted a 
migrated virtual machine to never be restarted, didn't you have a 
problem before my patch as well?

Note that autostart (the variable) is never read except to choose 
whether to do that vm_start; so there can be no collateral effects from 
passing -S.

I don't understand what this patch is trying to achieve (and why).

Paolo

[Qemu-devel] Re: [PATCH] fix broken migration

[Glauber Costa]

On Sat, Jul 25, 2009 at 03:37:24AM +0200, Paolo Bonzini wrote:
> On 07/24/2009 10:20 PM, Glauber Costa wrote:
>> While fixing migration with -S, commit
>> 89befdd1a6b18215153b8976682d57b7d03d5782 broke the rest of us.
>
> With this patch a migrated virtual machine will never be restarted.  If  
> you want that, shouldn't you use -S?  But if you really wanted a  
> migrated virtual machine to never be restarted, didn't you have a  
> problem before my patch as well?
>
> Note that autostart (the variable) is never read except to choose  
> whether to do that vm_start; so there can be no collateral effects from  
> passing -S.
>
> I don't understand what this patch is trying to achieve (and why).

If I pass --incoming to the VM, I don't expect the VM to start. I expect it
to wait, because I'll be handling it a migration later. However, it does start,
which is clearly wrong.

Of course, it does not start if we pass -S in conjunction with --incoming, but this
is a totally crazy semantics. An option should not depend on another option
to do the right thing.

As I said in the changelog, -S is still respected in migration because we're not
automatically vm_start()'in anything after migration completes, and also work
with a migrated machine that is already running. So what am I missing here?

[Qemu-devel] Re: [PATCH] fix broken migration

[Paolo Bonzini]

On 07/27/2009 08:02 PM, Glauber Costa wrote:
> If I pass --incoming to the VM, I don't expect the VM to start. I expect it
> to wait, because I'll be handling it a migration later. However, it does start,
> which is clearly wrong.

I see now, thanks.  I was puzzled by your message, so to speak.

I suppose the best semantics would be to do like

    if (autostart && !incoming)
        vm_start ()

in vl.c and add

    if (autostart)
        vm_start ()

in the migration-*.c files.

Paolo

[Qemu-devel] Re: [PATCH] fix broken migration

[Glauber Costa]

On Mon, Jul 27, 2009 at 09:31:43PM +0200, Paolo Bonzini wrote:
> On 07/27/2009 08:02 PM, Glauber Costa wrote:
>> If I pass --incoming to the VM, I don't expect the VM to start. I expect it
>> to wait, because I'll be handling it a migration later. However, it does start,
>> which is clearly wrong.
>
> I see now, thanks.  I was puzzled by your message, so to speak.
>
> I suppose the best semantics would be to do like
>
>    if (autostart && !incoming)
>        vm_start ()
>
> in vl.c and add
>
>    if (autostart)
>        vm_start ()
>
> in the migration-*.c files.
I don't disagree. This is even more explicit about the meaning of each of
the flags.

[Qemu-devel] [PATCH] fix migration to not require -S

[Paolo Bonzini]

Since migration returns right away, starting the VM right
after calling qemu_start_incoming_migration is wrong even
if -S is not passed.

Cc: Glauber Costa  <glommer@redhat.com>
Cc: Anthony Liguori  <aliguori@us.ibm.com>
---
	Oh my gosh.  This is what I have in my tree and I did not squash
	it before sending out the patch to the list.  I guess that this
	monstrosity :-) explains my reaction to Glauber's patch.

	Anthony, I don't care if you apply my patch or his.

 vl.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/vl.c b/vl.c
index 845d185..d826968 100644
--- a/vl.c
+++ b/vl.c
@@ -6198,7 +6198,7 @@ int main(int argc, char **argv, char **envp)
     if (incoming)
         qemu_start_incoming_migration(incoming);
 
-    if (autostart)
+    else if (autostart)
         vm_start();
 
 #ifndef _WIN32
-- 
1.6.2.5

[Qemu-devel] [PATCH alternative] fix migration to obey -S

[Paolo Bonzini]

Since migration returns right away, starting the VM right
after calling qemu_start_incoming_migration is wrong even
if -S is not passed.  We have to do this after migration
has completed.

Cc: Glauber Costa  <glommer@redhat.com>
Cc: Anthony Liguori  <aliguori@us.ibm.com>
---
	This implements the other semantics that glommer
	also agreed were nice to have.

 migration-exec.c |    2 ++
 migration-tcp.c  |    2 ++
 sysemu.h         |    1 +
 vl.c             |    4 ++--
 4 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/migration-exec.c b/migration-exec.c
index e472979..6249b93 100644
--- a/migration-exec.c
+++ b/migration-exec.c
@@ -118,6 +118,8 @@ static void exec_accept_incoming_migration(void *opaque)
     dprintf("successfully loaded vm state\n");
     /* we've successfully migrated, close the fd */
     qemu_set_fd_handler2(qemu_popen_fd(f), NULL, NULL, NULL, NULL);
+    if (autostart)
+        vm_start();
 
 err:
     qemu_fclose(f);
diff --git a/migration-tcp.c b/migration-tcp.c
index 7a87a1e..d3feb85 100644
--- a/migration-tcp.c
+++ b/migration-tcp.c
@@ -166,6 +166,8 @@ static void tcp_accept_incoming_migration(void *opaque)
     /* we've successfully migrated, close the server socket */
     qemu_set_fd_handler2(s, NULL, NULL, NULL, NULL);
     close(s);
+    if (autostart)
+        vm_start();
 
 out_fopen:
     qemu_fclose(f);
diff --git a/sysemu.h b/sysemu.h
index fe24415..9ab346c 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -102,6 +102,7 @@ typedef enum DisplayType
     DT_NOGRAPHIC,
 } DisplayType;
 
+extern int autostart;
 extern int bios_size;
 extern int cirrus_vga_enabled;
 extern int std_vga_enabled;
diff --git a/vl.c b/vl.c
index 845d185..02a8cd4 100644
--- a/vl.c
+++ b/vl.c
@@ -210,7 +210,7 @@ ram_addr_t ram_size;
 int nb_nics;
 NICInfo nd_table[MAX_NICS];
 int vm_running;
-static int autostart;
+int autostart;
 static int rtc_utc = 1;
 static int rtc_date_offset = -1; /* -1 means no change */
 int cirrus_vga_enabled = 1;
@@ -6198,7 +6198,7 @@ int main(int argc, char **argv, char **envp)
     if (incoming)
         qemu_start_incoming_migration(incoming);
 
-    if (autostart)
+    else if (autostart)
         vm_start();
 
 #ifndef _WIN32

[Qemu-devel] Re: [PATCH alternative] fix migration to obey -S

[Glauber Costa]

On Mon, Jul 27, 2009 at 11:17:51PM +0200, Paolo Bonzini wrote:
> Since migration returns right away, starting the VM right
> after calling qemu_start_incoming_migration is wrong even
> if -S is not passed.  We have to do this after migration
> has completed.
> 
> Cc: Glauber Costa  <glommer@redhat.com>
> Cc: Anthony Liguori  <aliguori@us.ibm.com>
> ---
> 	This implements the other semantics that glommer
> 	also agreed were nice to have.
> 
>  migration-exec.c |    2 ++
>  migration-tcp.c  |    2 ++
>  sysemu.h         |    1 +
>  vl.c             |    4 ++--
>  4 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/migration-exec.c b/migration-exec.c
> index e472979..6249b93 100644
> --- a/migration-exec.c
> +++ b/migration-exec.c
> @@ -118,6 +118,8 @@ static void exec_accept_incoming_migration(void *opaque)
>      dprintf("successfully loaded vm state\n");
>      /* we've successfully migrated, close the fd */
>      qemu_set_fd_handler2(qemu_popen_fd(f), NULL, NULL, NULL, NULL);
> +    if (autostart)
> +        vm_start();
>  
>  err:
>      qemu_fclose(f);
> diff --git a/migration-tcp.c b/migration-tcp.c
> index 7a87a1e..d3feb85 100644
> --- a/migration-tcp.c
> +++ b/migration-tcp.c
> @@ -166,6 +166,8 @@ static void tcp_accept_incoming_migration(void *opaque)
>      /* we've successfully migrated, close the server socket */
>      qemu_set_fd_handler2(s, NULL, NULL, NULL, NULL);
>      close(s);
> +    if (autostart)
> +        vm_start();
>  

Hummm,, those are a little bit weird. I'd expect it to be a characteristic of the
source machine, no the destination. IOW, if the machine was running prior to migration,
it should be running after it, and if it was stopped prior to migration, it should be
stopped after it.

Having dst flags to tamper this seems only confusing to me.

[Qemu-devel] Re: [PATCH alternative] fix migration to obey -S

[Anthony Liguori]

Glauber Costa wrote:
> Hummm,, those are a little bit weird. I'd expect it to be a characteristic of the
> source machine, no the destination. IOW, if the machine was running prior to migration,
> it should be running after it, and if it was stopped prior to migration, it should be
> stopped after it.
>   

Whether a guest is running is not part of it's state-IOW, it's not 
visible to the guest whether it's running or not.

-- 
Regards,

Anthony Liguori

[Qemu-devel] Re: [PATCH alternative] fix migration to obey -S

[Anthony Liguori]

Glauber Costa wrote:
> On Mon, Jul 27, 2009 at 04:36:53PM -0500, Anthony Liguori wrote:
>   
>> Glauber Costa wrote:
>>     
>>> Hummm,, those are a little bit weird. I'd expect it to be a characteristic of the
>>> source machine, no the destination. IOW, if the machine was running prior to migration,
>>> it should be running after it, and if it was stopped prior to migration, it should be
>>> stopped after it.
>>>   
>>>       
>> Whether a guest is running is not part of it's state-IOW, it's not  
>> visible to the guest whether it's running or not.
>>     
> Can't we then register a savevm function for that?
>   

No, that's the point.  Since it's not guest visible, it should not be 
saved as part of savevm.

It's up to the management tools to keep track of whether a VM is stopped 
or not.  When creating the destination node, the user specify whether 
they want the VM to be stopped.

So qemu -incoming ... -S

Should translate to: do not automatically run the VM after migration.

qemu -incoming ...

Will always run the VM after migration regardless of whether the VM was 
migrated while stopped.

-- 
Regards,

Anthony Liguori

[Qemu-devel] Re: [PATCH alternative] fix migration to obey -S

[Glauber Costa]

On Mon, Jul 27, 2009 at 04:36:53PM -0500, Anthony Liguori wrote:
> Glauber Costa wrote:
>> Hummm,, those are a little bit weird. I'd expect it to be a characteristic of the
>> source machine, no the destination. IOW, if the machine was running prior to migration,
>> it should be running after it, and if it was stopped prior to migration, it should be
>> stopped after it.
>>   
>
> Whether a guest is running is not part of it's state-IOW, it's not  
> visible to the guest whether it's running or not.
Can't we then register a savevm function for that?
Otherwise, management gets quite complicated, if they really want to get the behaviour
I described (which I believe to be the most reasonable one)