[PATCH]: Make migration handle errors a little better

[PATCH]: Make migration handle errors a little better

[Chris Lalancette]

[-- Attachment #1: Type: text/plain, Size: 516 bytes --]

All,
     Attached is a fairly simple patch to the migration code to make it handle
errors better.  In particular, if the remote side wasn't set up properly (i.e.
the memory size didn't match), the migration would fail, but the source host
didn't actually react properly, so the migration would still be "in-progress".
This patch fixes up the error checking to properly quit when the remote side
isn't prepared for whatever reason.

Signed-off-by: Chris Lalancette <clalance-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

[-- Attachment #2: kvm-60-migration-error-checking.patch --]
[-- Type: text/x-patch, Size: 2261 bytes --]

diff --git a/qemu/migration.c b/qemu/migration.c
index 17ad77c..ae95128 100644
--- a/qemu/migration.c
+++ b/qemu/migration.c
@@ -86,6 +86,7 @@ enum { /* migration status values */
 
     MIG_STAT_SAVEVM_FAILED     = 15,
     MIG_STAT_NO_MEM            = 16,
+    MIG_STAT_DIRTY_TRACK_FAIL  = 17,
 
     MIG_STAT_MIGRATION_CANCEL  = 20,
 
@@ -425,18 +426,26 @@ static int start_migration(MigrationState *s)
 #endif
 	
     r = MIG_STAT_WRITE_FAILED;
-    if (write_whole_buffer(s->fd, &running, sizeof(running)))
+    if (write_whole_buffer(s->fd, &running, sizeof(running))) {
+        perror("vm_running write failed");
         goto out;
-    if (write_whole_buffer(s->fd, &value, sizeof(value)))
+    }
+    if (write_whole_buffer(s->fd, &value, sizeof(value))) {
+        perror("phys_ram_size write failed");
         goto out;
+    }
 
 #ifdef USE_KVM
     if (kvm_allowed) {
         value = cpu_to_be32(n);
-        if (write_whole_buffer(s->fd, &value, sizeof(value)))
+        if (write_whole_buffer(s->fd, &value, sizeof(value))) {
+            perror("phys_ram_size_bitmap size write failed");
             goto out;
-        if (write_whole_buffer(s->fd, phys_ram_page_exist_bitmap, n))
+	}
+        if (write_whole_buffer(s->fd, phys_ram_page_exist_bitmap, n)) {
+            perror("phys_ram_page_exist_bitmap write failed");
             goto out;
+	}
     }
 #endif
     fcntl(s->fd, F_SETFL, O_NONBLOCK);
@@ -456,7 +465,8 @@ static int start_migration(MigrationState *s)
 
     if (cpu_physical_memory_set_dirty_tracking(1)) {
         *s->has_error = MIG_STAT_KVM_SET_DIRTY_TRACKING_FAILED;
-        return -1;
+        r = MIG_STAT_DIRTY_TRACK_FAIL;
+        goto out;
     }
 
     s->addr = 0;
@@ -470,6 +480,8 @@ static int start_migration(MigrationState *s)
     qemu_mod_timer(s->timer, qemu_get_clock(rt_clock));
     qemu_set_fd_handler2(s->fd, NULL, NULL, migrate_write, s);
 
+    r = 0;
+
  out:
 #ifdef USE_KVM
     if (phys_ram_page_exist_bitmap)
@@ -498,8 +510,9 @@ static MigrationState *migration_init_fd(int detach, int fd)
 
     current_migration = s;
     
-    if (start_migration(s) == -1) {
+    if (start_migration(s)) {
 	term_printf("Could not start migration\n");
+        migrate_close(s);
 	return NULL;
     }
 

[-- Attachment #3: Type: text/plain, Size: 228 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

[-- Attachment #4: Type: text/plain, Size: 186 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Re: [PATCH]: Make migration handle errors a little better

[Uri Lublin]

Chris Lalancette wrote:
> All,
>      Attached is a fairly simple patch to the migration code to make it handle
> errors better.  In particular, if the remote side wasn't set up properly (i.e.
> the memory size didn't match), the migration would fail, but the source host
> didn't actually react properly, so the migration would still be "in-progress".
> This patch fixes up the error checking to properly quit when the remote side
> isn't prepared for whatever reason.
>
> Signed-off-by: Chris Lalancette <clalance-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>   
Patch looks good.
Why did you define MIG_STAT_DIRTY_TRACK_FAIL and not 
MIG_STAT_KVM_SET_DIRTY_TRACKING_FAILED ?
Also we need to make sure, in case of a failure, that 'status' is  
updated and *s->has_error is freed. How about calling migrate_finish ?
Thanks,
Uri.
 

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH]: Make migration handle errors a little better

[Chris Lalancette]

Uri Lublin wrote:
> 
> Chris Lalancette wrote:
>> All,
>>      Attached is a fairly simple patch to the migration code to make it handle
>> errors better.  In particular, if the remote side wasn't set up properly (i.e.
>> the memory size didn't match), the migration would fail, but the source host
>> didn't actually react properly, so the migration would still be "in-progress".
>> This patch fixes up the error checking to properly quit when the remote side
>> isn't prepared for whatever reason.
>>
>> Signed-off-by: Chris Lalancette <clalance-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>   
> Patch looks good.
> Why did you define MIG_STAT_DIRTY_TRACK_FAIL and not 
> MIG_STAT_KVM_SET_DIRTY_TRACKING_FAILED ?

Heh.  I was just being lazy, and I didn't want to type all of that :).  I can
change it to be more correct, though.

> Also we need to make sure, in case of a failure, that 'status' is  
> updated and *s->has_error is freed. How about calling migrate_finish ?

Oh, good point.  I thought calling migrate_close() would be enough, but it looks
like you are right, we also need to free up has_error.  I'll send an updated patch.

Thanks for the review!
Chris Lalancette

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH]: Make migration handle errors a little better

[Chris Lalancette]

[-- Attachment #1: Type: text/plain, Size: 683 bytes --]

Uri Lublin wrote:
> Patch looks good.
> Why did you define MIG_STAT_DIRTY_TRACK_FAIL and not 
> MIG_STAT_KVM_SET_DIRTY_TRACKING_FAILED ?

Oops, I slightly misunderstood this bit in my last e-mail.  You were saying that
there is already a "MIG_STAT_KVM_SET_DIRTY_TRACKING_FAILED" error flag.  In the
new patch (attached), I've used this instead.

> Also we need to make sure, in case of a failure, that 'status' is  
> updated and *s->has_error is freed. How about calling migrate_finish ?

And I now call migrate_finish() instead of migrate_close() in the error path, as
you suggest.  Thanks again!

Signed-off-by: Chris Lalancette <clalance-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

[-- Attachment #2: kvm-60-migration-error-checking2.patch --]
[-- Type: text/x-patch, Size: 2055 bytes --]

diff --git a/qemu/migration.c b/qemu/migration.c
index 17ad77c..df0acf9 100644
--- a/qemu/migration.c
+++ b/qemu/migration.c
@@ -425,18 +425,26 @@ static int start_migration(MigrationState *s)
 #endif
 	
     r = MIG_STAT_WRITE_FAILED;
-    if (write_whole_buffer(s->fd, &running, sizeof(running)))
+    if (write_whole_buffer(s->fd, &running, sizeof(running))) {
+        perror("vm_running write failed");
         goto out;
-    if (write_whole_buffer(s->fd, &value, sizeof(value)))
+    }
+    if (write_whole_buffer(s->fd, &value, sizeof(value))) {
+        perror("phys_ram_size write failed");
         goto out;
+    }
 
 #ifdef USE_KVM
     if (kvm_allowed) {
         value = cpu_to_be32(n);
-        if (write_whole_buffer(s->fd, &value, sizeof(value)))
+        if (write_whole_buffer(s->fd, &value, sizeof(value))) {
+            perror("phys_ram_size_bitmap size write failed");
             goto out;
-        if (write_whole_buffer(s->fd, phys_ram_page_exist_bitmap, n))
+	}
+        if (write_whole_buffer(s->fd, phys_ram_page_exist_bitmap, n)) {
+            perror("phys_ram_page_exist_bitmap write failed");
             goto out;
+	}
     }
 #endif
     fcntl(s->fd, F_SETFL, O_NONBLOCK);
@@ -456,7 +464,8 @@ static int start_migration(MigrationState *s)
 
     if (cpu_physical_memory_set_dirty_tracking(1)) {
         *s->has_error = MIG_STAT_KVM_SET_DIRTY_TRACKING_FAILED;
-        return -1;
+        r = MIG_STAT_KVM_SET_DIRTY_TRACKING_FAILED;
+        goto out;
     }
 
     s->addr = 0;
@@ -470,6 +479,8 @@ static int start_migration(MigrationState *s)
     qemu_mod_timer(s->timer, qemu_get_clock(rt_clock));
     qemu_set_fd_handler2(s->fd, NULL, NULL, migrate_write, s);
 
+    r = 0;
+
  out:
 #ifdef USE_KVM
     if (phys_ram_page_exist_bitmap)
@@ -498,8 +509,9 @@ static MigrationState *migration_init_fd(int detach, int fd)
 
     current_migration = s;
     
-    if (start_migration(s) == -1) {
+    if (start_migration(s)) {
 	term_printf("Could not start migration\n");
+	migrate_finish(s);
 	return NULL;
     }
 

[-- Attachment #3: Type: text/plain, Size: 228 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

[-- Attachment #4: Type: text/plain, Size: 186 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Re: [PATCH]: Make migration handle errors a little better

[Avi Kivity]

Chris Lalancette wrote:
> Uri Lublin wrote:
>   
>> Patch looks good.
>> Why did you define MIG_STAT_DIRTY_TRACK_FAIL and not 
>> MIG_STAT_KVM_SET_DIRTY_TRACKING_FAILED ?
>>     
>
> Oops, I slightly misunderstood this bit in my last e-mail.  You were saying that
> there is already a "MIG_STAT_KVM_SET_DIRTY_TRACKING_FAILED" error flag.  In the
> new patch (attached), I've used this instead.
>
>   

Applied; thanks.

-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/