[Qemu-devel] [PATCH 0/6] Refactor autostart of machines

[Qemu-devel] [PATCH 0/6] Refactor autostart of machines

[Juan Quintela]

Hi
	This series add several things:
- delay sighandler_setup().  Current code hangs until you are using
  telnet:....,server for instance.  With this change, ^C while waiting
  for telnet program to conect will kill the vm.

- refactor autostart behaviour:
  * incoming migration: depend of -S parameter
  * loadvm from command line: depend of -S parameter
  * loadvm from monitor: same state that the vm it replaces
  * no loadvm no incoming: depend of -S parameter
  I tested, and all combinations works this way.

- refactor do_loadvm().  do_loadvm() don't return errors, if there were
  any error while loading a vm, it would obey autostart value independently of
  the errors.  Now it will never start a vm that had erros while loading.

- if you type "cont" in the monitor of one machine just migrated, qemu will
  proceed with the start, with the consequent corruption.  Last patch forbides
  to continue a vm that has just been migrated with an error message.  If you
  load a new vm, flag is reset.  This "bug/feature" was reported as a bug
  by users.

One comment:
- Is there really a good idea to allow loadvm in the monitor even if the current
  vm is running?  I think that allowing loadvm command to only works when vm is
  stopped is a better idea.  what does everybody else thinks?

Comments?

Later, Juan.

Juan Quintela (6):
  Delay sighandler_setup()
  We want to autostart on incoming conections Once there, indent
    surrounded code in qemu style
  split do_loadvm() into do_loadvm() and load_vmstate()
  move do_loadvm() to monitor.c
  make load_vmstate() return errors
  Continue a migrated vm is a bad idea

 migration.c |    1 +
 monitor.c   |   16 ++++++++++++++++
 savevm.c    |   19 +++++++------------
 sysemu.h    |    3 ++-
 vl.c        |   24 +++++++++++++-----------
 5 files changed, 39 insertions(+), 24 deletions(-)

[Qemu-devel] [PATCH 1/6] Delay sighandler_setup()

[Juan Quintela]

If we are using --serial telnet:0:5555,server  or similar, ^C will not
kill qemu.  We need to first connect using telnet, and the the ^C takes
effect.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 vl.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/vl.c b/vl.c
index 8b2b289..66246af 100644
--- a/vl.c
+++ b/vl.c
@@ -5789,11 +5789,6 @@ int main(int argc, char **argv, char **envp)
     register_savevm("timer", 0, 2, timer_save, timer_load, NULL);
     register_savevm_live("ram", 0, 3, ram_save_live, NULL, ram_load, NULL);

-#ifndef _WIN32
-    /* must be after terminal init, SDL library changes signal handlers */
-    sighandler_setup();
-#endif
-
     /* Maintain compatibility with multiple stdio monitors */
     if (!strcmp(monitor_device,"stdio")) {
         for (i = 0; i < MAX_SERIAL_PORTS; i++) {
@@ -5920,6 +5915,11 @@ int main(int argc, char **argv, char **envp)
                   kernel_filename, kernel_cmdline, initrd_filename, cpu_model);


+#ifndef _WIN32
+    /* must be after terminal init, SDL library changes signal handlers */
+    sighandler_setup();
+#endif
+
     for (env = first_cpu; env != NULL; env = env->next_cpu) {
         for (i = 0; i < nb_numa_nodes; i++) {
             if (node_cpumask[i] & (1 << env->cpu_index)) {
-- 
1.6.2.5

[Qemu-devel] [PATCH 2/6] We want to autostart on incoming conections Once there, indent surrounded code in qemu style

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 vl.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/vl.c b/vl.c
index 66246af..e86bdce 100644
--- a/vl.c
+++ b/vl.c
@@ -6040,12 +6040,10 @@ int main(int argc, char **argv, char **envp)
         do_loadvm(cur_mon, loadvm);

     if (incoming) {
-        autostart = 0;
         qemu_start_incoming_migration(incoming);
-    }
-
-    else if (autostart)
+    } else if (autostart) {
         vm_start();
+    }

 #ifndef _WIN32
     if (daemonize) {
-- 
1.6.2.5

[Qemu-devel] Re: [PATCH 2/6] We want to autostart on incoming conections Once there, indent surrounded code in qemu style

[Paolo Bonzini]

On 08/19/2009 04:07 AM, Juan Quintela wrote:
> Signed-off-by: Juan Quintela<quintela@redhat.com>
> ---
>   vl.c |    6 ++----
>   1 files changed, 2 insertions(+), 4 deletions(-)

This is effectively reverting 2bb8c10c (more or less) and is already in 
Anthony's queue.

Paolo

[Qemu-devel] Re: [PATCH 2/6] We want to autostart on incoming conections Once there, indent surrounded code in qemu style

[Juan Quintela]

Reply-to: quintela@redhat.com
Paolo Bonzini <bonzini@gnu.org> wrote:
> On 08/19/2009 04:07 AM, Juan Quintela wrote:
>> Signed-off-by: Juan Quintela<quintela@redhat.com>
>> ---
>>   vl.c |    6 ++----
>>   1 files changed, 2 insertions(+), 4 deletions(-)
>
> This is effectively reverting 2bb8c10c (more or less) and is already
> in Anthony's queue.

But the series make everything work as expected and consistently, just
now it is a mess :(

Later, Juan.

[Qemu-devel] Re: [PATCH 2/6] We want to autostart on incoming conections Once there, indent surrounded code in qemu style

[Paolo Bonzini]

On 08/19/2009 11:18 AM, Juan Quintela wrote:
> Reply-to: quintela@redhat.com
> Paolo Bonzini<bonzini@gnu.org>  wrote:
>> On 08/19/2009 04:07 AM, Juan Quintela wrote:
>>> Signed-off-by: Juan Quintela<quintela@redhat.com>
>>> ---
>>>    vl.c |    6 ++----
>>>    1 files changed, 2 insertions(+), 4 deletions(-)
>>
>> This is effectively reverting 2bb8c10c (more or less) and is already
>> in Anthony's queue.
>
> But the series make everything work as expected and consistently, just
> now it is a mess :(

I was not saying it is wrong, just that it is unnecessary when applied 
on aliguori-queue.git. :-)

Paolo

[Qemu-devel] [PATCH 3/6] split do_loadvm() into do_loadvm() and load_vmstate()

[Juan Quintela]

do_loadvm() is now called from the monitor.
load_vmstate() is called by do_loadvm() and when -loadvm command line is used.
Command line don't have to play games with vmstop()/vmstart()

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 savevm.c |   22 +++++++++++++---------
 sysemu.h |    1 +
 vl.c     |    2 +-
 3 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/savevm.c b/savevm.c
index 570377f..a321136 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1145,14 +1145,13 @@ void do_savevm(Monitor *mon, const char *name)
         vm_start();
 }

-void do_loadvm(Monitor *mon, const char *name)
+void load_vmstate(Monitor *mon, const char *name)
 {
     DriveInfo *dinfo;
     BlockDriverState *bs, *bs1;
     QEMUSnapshotInfo sn;
     QEMUFile *f;
     int ret;
-    int saved_vm_running;

     bs = get_bs_snapshots();
     if (!bs) {
@@ -1163,9 +1162,6 @@ void do_loadvm(Monitor *mon, const char *name)
     /* Flush all IO requests so they don't interfere with the new state.  */
     qemu_aio_flush();

-    saved_vm_running = vm_running;
-    vm_stop(0);
-
     TAILQ_FOREACH(dinfo, &drives, next) {
         bs1 = dinfo->bdrv;
         if (bdrv_has_snapshot(bs1)) {
@@ -1191,7 +1187,7 @@ void do_loadvm(Monitor *mon, const char *name)
                 }
                 /* fatal on snapshot block device */
                 if (bs == bs1)
-                    goto the_end;
+                    return;
             }
         }
     }
@@ -1199,20 +1195,28 @@ void do_loadvm(Monitor *mon, const char *name)
     /* Don't even try to load empty VM states */
     ret = bdrv_snapshot_find(bs, &sn, name);
     if ((ret >= 0) && (sn.vm_state_size == 0))
-        goto the_end;
+        return;

     /* restore the VM state */
     f = qemu_fopen_bdrv(bs, 0);
     if (!f) {
         monitor_printf(mon, "Could not open VM state file\n");
-        goto the_end;
+        return;
     }
     ret = qemu_loadvm_state(f);
     qemu_fclose(f);
     if (ret < 0) {
         monitor_printf(mon, "Error %d while loading VM state\n", ret);
     }
- the_end:
+}
+
+void do_loadvm(Monitor *mon, const char *name)
+{
+    int saved_vm_running  = vm_running;
+
+    vm_stop(0);
+
+    load_vmstate(mon, name);
     if (saved_vm_running)
         vm_start();
 }
diff --git a/sysemu.h b/sysemu.h
index dffb2f1..a20025d 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -52,6 +52,7 @@ void qemu_system_reset(void);

 void do_savevm(Monitor *mon, const char *name);
 void do_loadvm(Monitor *mon, const char *name);
+void load_vmstate(Monitor *mon, const char *name);
 void do_delvm(Monitor *mon, const char *name);
 void do_info_snapshots(Monitor *mon);

diff --git a/vl.c b/vl.c
index e86bdce..4d585b0 100644
--- a/vl.c
+++ b/vl.c
@@ -6037,7 +6037,7 @@ int main(int argc, char **argv, char **envp)
     }

     if (loadvm)
-        do_loadvm(cur_mon, loadvm);
+        load_vmstate(cur_mon, loadvm);

     if (incoming) {
         qemu_start_incoming_migration(incoming);
-- 
1.6.2.5

[Qemu-devel] [PATCH 4/6] move do_loadvm() to monitor.c

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 monitor.c |   11 +++++++++++
 savevm.c  |   11 -----------
 sysemu.h  |    1 -
 3 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/monitor.c b/monitor.c
index 362322b..b8a47ca 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1776,6 +1776,17 @@ static void do_closefd(Monitor *mon, const char *fdname)
                    fdname);
 }

+static void do_loadvm(Monitor *mon, const char *name)
+{
+    int saved_vm_running  = vm_running;
+
+    vm_stop(0);
+
+    load_vmstate(mon, name);
+    if (saved_vm_running)
+        vm_start();
+}
+
 int monitor_get_fd(Monitor *mon, const char *fdname)
 {
     mon_fd_t *monfd;
diff --git a/savevm.c b/savevm.c
index a321136..d0ed2ad 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1210,17 +1210,6 @@ void load_vmstate(Monitor *mon, const char *name)
     }
 }

-void do_loadvm(Monitor *mon, const char *name)
-{
-    int saved_vm_running  = vm_running;
-
-    vm_stop(0);
-
-    load_vmstate(mon, name);
-    if (saved_vm_running)
-        vm_start();
-}
-
 void do_delvm(Monitor *mon, const char *name)
 {
     DriveInfo *dinfo;
diff --git a/sysemu.h b/sysemu.h
index a20025d..330386c 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -51,7 +51,6 @@ extern qemu_irq qemu_system_powerdown;
 void qemu_system_reset(void);

 void do_savevm(Monitor *mon, const char *name);
-void do_loadvm(Monitor *mon, const char *name);
 void load_vmstate(Monitor *mon, const char *name);
 void do_delvm(Monitor *mon, const char *name);
 void do_info_snapshots(Monitor *mon);
-- 
1.6.2.5

[Qemu-devel] [PATCH 5/6] make load_vmstate() return errors

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 monitor.c |    3 +--
 savevm.c  |   12 +++++++-----
 sysemu.h  |    2 +-
 vl.c      |    7 +++++--
 4 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/monitor.c b/monitor.c
index b8a47ca..ea5c33a 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1782,8 +1782,7 @@ static void do_loadvm(Monitor *mon, const char *name)

     vm_stop(0);

-    load_vmstate(mon, name);
-    if (saved_vm_running)
+    if (load_vmstate(mon, name) >= 0 && saved_vm_running)
         vm_start();
 }

diff --git a/savevm.c b/savevm.c
index d0ed2ad..4868285 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1145,7 +1145,7 @@ void do_savevm(Monitor *mon, const char *name)
         vm_start();
 }

-void load_vmstate(Monitor *mon, const char *name)
+int load_vmstate(Monitor *mon, const char *name)
 {
     DriveInfo *dinfo;
     BlockDriverState *bs, *bs1;
@@ -1156,7 +1156,7 @@ void load_vmstate(Monitor *mon, const char *name)
     bs = get_bs_snapshots();
     if (!bs) {
         monitor_printf(mon, "No block device supports snapshots\n");
-        return;
+        return -EINVAL;
     }

     /* Flush all IO requests so they don't interfere with the new state.  */
@@ -1187,7 +1187,7 @@ void load_vmstate(Monitor *mon, const char *name)
                 }
                 /* fatal on snapshot block device */
                 if (bs == bs1)
-                    return;
+                    return 0;
             }
         }
     }
@@ -1195,19 +1195,21 @@ void load_vmstate(Monitor *mon, const char *name)
     /* Don't even try to load empty VM states */
     ret = bdrv_snapshot_find(bs, &sn, name);
     if ((ret >= 0) && (sn.vm_state_size == 0))
-        return;
+        return -EINVAL;

     /* restore the VM state */
     f = qemu_fopen_bdrv(bs, 0);
     if (!f) {
         monitor_printf(mon, "Could not open VM state file\n");
-        return;
+        return -EINVAL;
     }
     ret = qemu_loadvm_state(f);
     qemu_fclose(f);
     if (ret < 0) {
         monitor_printf(mon, "Error %d while loading VM state\n", ret);
+        return ret;
     }
+    return 0;
 }

 void do_delvm(Monitor *mon, const char *name)
diff --git a/sysemu.h b/sysemu.h
index 330386c..e0338ce 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -51,7 +51,7 @@ extern qemu_irq qemu_system_powerdown;
 void qemu_system_reset(void);

 void do_savevm(Monitor *mon, const char *name);
-void load_vmstate(Monitor *mon, const char *name);
+int load_vmstate(Monitor *mon, const char *name);
 void do_delvm(Monitor *mon, const char *name);
 void do_info_snapshots(Monitor *mon);

diff --git a/vl.c b/vl.c
index 4d585b0..bf2468e 100644
--- a/vl.c
+++ b/vl.c
@@ -6036,8 +6036,11 @@ int main(int argc, char **argv, char **envp)
         exit(1);
     }

-    if (loadvm)
-        load_vmstate(cur_mon, loadvm);
+    if (loadvm) {
+        if (load_vmstate(cur_mon, loadvm) < 0) {
+            autostart = 0;
+        }
+    }

     if (incoming) {
         qemu_start_incoming_migration(incoming);
-- 
1.6.2.5

[Qemu-devel] [PATCH 6/6] Continue a migrated vm is a bad idea

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration.c |    1 +
 monitor.c   |    6 ++++++
 sysemu.h    |    1 +
 vl.c        |    1 +
 4 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/migration.c b/migration.c
index ee64d41..f91401e 100644
--- a/migration.c
+++ b/migration.c
@@ -275,6 +275,7 @@ void migrate_fd_put_ready(void *opaque)
             state = MIG_STATE_ERROR;
         } else {
             state = MIG_STATE_COMPLETED;
+            vm_has_migrated = 1;
         }
         migrate_fd_cleanup(s);
         s->state = state;
diff --git a/monitor.c b/monitor.c
index ea5c33a..762a9a5 100644
--- a/monitor.c
+++ b/monitor.c
@@ -567,6 +567,11 @@ static void do_cont(Monitor *mon)
 {
     struct bdrv_iterate_context context = { mon, 0 };

+    if (unlikely(vm_has_migrated)) {
+        monitor_printf(mon, "this vm has migrated to other machine\n");
+        monitor_printf(mon, "You have to load other vm\n");
+        return;
+    }
     bdrv_iterate(encrypted_bdrv_it, &context);
     /* only resume the vm if all keys are set and valid */
     if (!context.err)
@@ -1780,6 +1785,7 @@ static void do_loadvm(Monitor *mon, const char *name)
 {
     int saved_vm_running  = vm_running;

+    vm_has_migrated = 0;
     vm_stop(0);

     if (load_vmstate(mon, name) >= 0 && saved_vm_running)
diff --git a/sysemu.h b/sysemu.h
index e0338ce..7c3fdc1 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -18,6 +18,7 @@ extern const char *bios_name;
 char *qemu_find_file(int type, const char *name);

 extern int vm_running;
+extern int vm_has_migrated;
 extern const char *qemu_name;
 extern uint8_t qemu_uuid[];
 int qemu_uuid_parse(const char *str, uint8_t *uuid);
diff --git a/vl.c b/vl.c
index bf2468e..d39295a 100644
--- a/vl.c
+++ b/vl.c
@@ -188,6 +188,7 @@ ram_addr_t ram_size;
 int nb_nics;
 NICInfo nd_table[MAX_NICS];
 int vm_running;
+int vm_has_migrated;
 int autostart;
 static int rtc_utc = 1;
 static int rtc_date_offset = -1; /* -1 means no change */
-- 
1.6.2.5

Re: [Qemu-devel] [PATCH 6/6] Continue a migrated vm is a bad idea

[Avi Kivity]

On 08/19/2009 05:07 AM, Juan Quintela wrote:
> index ea5c33a..762a9a5 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -567,6 +567,11 @@ static void do_cont(Monitor *mon)
>   {
>       struct bdrv_iterate_context context = { mon, 0 };
>
> +    if (unlikely(vm_has_migrated)) {
> +        monitor_printf(mon, "this vm has migrated to other machine\n");
> +        monitor_printf(mon, "You have to load other vm\n");
> +        return;
> +    }
>    

You don't know whether the guest is running on the other side.  It may 
be that the control program started the other side stopped, and after 
migration completes decides which side to continue running.

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

[Qemu-devel] Re: [PATCH 6/6] Continue a migrated vm is a bad idea

[Juan Quintela]

Avi Kivity <avi@redhat.com> wrote:
> On 08/19/2009 05:07 AM, Juan Quintela wrote:
>> index ea5c33a..762a9a5 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -567,6 +567,11 @@ static void do_cont(Monitor *mon)
>>   {
>>       struct bdrv_iterate_context context = { mon, 0 };
>>
>> +    if (unlikely(vm_has_migrated)) {
>> +        monitor_printf(mon, "this vm has migrated to other machine\n");
>> +        monitor_printf(mon, "You have to load other vm\n");
>> +        return;
>> +    }
>>    
>
> You don't know whether the guest is running on the other side.  It may
> be that the control program started the other side stopped, and after
> migration completes decides which side to continue running.

Only works if migration suceeded and was done in _non_ autostart node.
Have to debug this very bug.  User thought that he had done _nothing_
on the destination side, and that then stoping it, and continue in the
source side was a good idea.

He didn't understand why he had disk corruption, obviosly qemu had a
bug.  I haven't found a better approach to fix this bug.  If you have a
better idea of what to do in this case, I am all ears.

The reason that I created the patch is that if the user didn't got it
right, it gets disk corruption.

Later, Juan.

[Qemu-devel] Re: [PATCH 6/6] Continue a migrated vm is a bad idea

[Avi Kivity]

On 08/20/2009 12:10 PM, Juan Quintela wrote:
> Avi Kivity<avi@redhat.com>  wrote:
>    
>> On 08/19/2009 05:07 AM, Juan Quintela wrote:
>>      
>>> index ea5c33a..762a9a5 100644
>>> --- a/monitor.c
>>> +++ b/monitor.c
>>> @@ -567,6 +567,11 @@ static void do_cont(Monitor *mon)
>>>    {
>>>        struct bdrv_iterate_context context = { mon, 0 };
>>>
>>> +    if (unlikely(vm_has_migrated)) {
>>> +        monitor_printf(mon, "this vm has migrated to other machine\n");
>>> +        monitor_printf(mon, "You have to load other vm\n");
>>> +        return;
>>> +    }
>>>
>>>        
>> You don't know whether the guest is running on the other side.  It may
>> be that the control program started the other side stopped, and after
>> migration completes decides which side to continue running.
>>      
> Only works if migration suceeded and was done in _non_ autostart node.
>    

Well, you're disabling this method.

> Have to debug this very bug.  User thought that he had done _nothing_
> on the destination side, and that then stoping it, and continue in the
> source side was a good idea.
>
> He didn't understand why he had disk corruption, obviosly qemu had a
> bug.  I haven't found a better approach to fix this bug.  If you have a
> better idea of what to do in this case, I am all ears.
>
> The reason that I created the patch is that if the user didn't got it
> right, it gets disk corruption.
>    

There are tons of ways to cause disk corruption running multiple qemus.  
This only solves one of them.

I suggest taking locks on the disk images instead (and adding an option 
not to lock for raw images).

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