Refactor autostart of machines
cover Refactor autostart of machines
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(-)
PATCH 1/6 Delay sighandler_setup()
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.
--- vl.c | 10 +++++----- 1 files changed, 5 insertions(+), 5 deletions(-)
vl.c +5 -5
@@ -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)) {
Signature · 1 line
1.6.2.5
PATCH 2/6 We want to autostart on incoming conections Once there, indent surrounded code in qemu style
In reply to Juan Quintela--- vl.c | 6 ++---- 1 files changed, 2 insertions(+), 4 deletions(-)
vl.c +2 -4
@@ -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 _WIN32if (daemonize) {
Signature · 1 line
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
Quoted 4 lines · Juan Quintela
Signed-off-by Juan Quintela<[email protected]>--- 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
Reply-to: [email protected]
Quoted 8 lines · Paolo Bonzini
Quoted 4 lines · Juan Quintela
Signed-off-by Juan Quintela<[email protected]>--- 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
Quoted 8 lines · Juan Quintela
Reply-to: [email protected]
Quoted 6 lines · Paolo Bonzini
Quoted 4 lines · Juan Quintela
Signed-off-by Juan Quintela<[email protected]>--- vl.c | 6 ++---- 1 files changed, 2 insertions(+), 4 deletions(-)
Quoted 5 lines
Quoted 2 lines
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
PATCH 3/6 split do_loadvm() into do_loadvm() and load_vmstate()
In reply to Juan Quintelado_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()
--- savevm.c | 22 +++++++++++++--------- sysemu.h | 1 + vl.c | 2 +- 3 files changed, 15 insertions(+), 10 deletions(-)
savevm.c +13 -9
@@ -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 (); }
sysemu.h +1 -0
@@ -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);
vl.c +1 -1
@@ -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);
Signature · 1 line
1.6.2.5
PATCH 4/6 move do_loadvm() to monitor.c
In reply to Juan Quintela--- monitor.c | 11 +++++++++++ savevm.c | 11 ----------- sysemu.h | 1 - 3 files changed, 11 insertions(+), 12 deletions(-)
monitor.c +11 -0
@@ -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;
savevm.c +0 -11
@@ -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;
sysemu.h +0 -1
@@ -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);
Signature · 1 line
1.6.2.5
PATCH 5/6 make load_vmstate() return errors
In reply to Juan Quintela--- monitor.c | 3 +-- savevm.c | 12 +++++++----- sysemu.h | 2 +- vl.c | 7 +++++-- 4 files changed, 14 insertions(+), 10 deletions(-)
monitor.c +1 -2
@@ -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 (); }
savevm.c +7 -5
@@ -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)
sysemu.h +1 -1
@@ -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);
vl.c +5 -2
@@ -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);
Signature · 1 line
1.6.2.5
PATCH 6/6 Continue a migrated vm is a bad idea
In reply to Juan Quintela--- migration.c | 1 + monitor.c | 6 ++++++ sysemu.h | 1 + vl.c | 1 + 4 files changed, 9 insertions(+), 0 deletions(-)
migration.c +1 -0
@@ -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;
monitor.c +6 -0
@@ -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)
sysemu.h +1 -0
@@ -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);
vl.c +1 -0
@@ -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 */
Signature · 1 line
1.6.2.5
Re: [Qemu-devel] [PATCH 6/6] Continue a migrated vm is a bad idea
monitor.c static void do_cont(Monitor *mon) · line 575
Quoted 13 lines · Juan Quintela
index ea5c33a..762a9a5 100644
monitor.c +5
-0@@ -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.
Signature · 1 line
error compiling committee.c: too many arguments to function
[Qemu-devel] Re: [PATCH 6/6] Continue a migrated vm is a bad idea
Quoted 8 lines · Avi Kivity
Quoted 7 lines · Juan Quintela
index ea5c33a..762a9a5 100644
monitor.c +0
-0@@ -567,6 +567,11 @@ static void do_cont(Monitor *mon) {struct bdrv_iterate_context context = { mon,0 };
Quoted 10 lines
monitor.c static void do_cont(Monitor *mon)
Quoted 6 lines
monitor.c +5
-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
Quoted 22 lines · Juan Quintela
Quoted 9 lines · Avi Kivity
Quoted 7 lines · Juan Quintela
index ea5c33a..762a9a5 100644
monitor.c +0
-0@@ -567,6 +567,11 @@ static void do_cont(Monitor *mon) {struct bdrv_iterate_context context = { mon,0 };Quoted 11 lines
monitor.c static void do_cont(Monitor *mon)
Quoted 7 lines
monitor.c +5
-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.
Quoted 2 lines
Only works if migration suceeded and was done in _non_ autostart node.
Well, you're disabling this method.
Quoted 11 lines
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).
Signature · 1 line
error compiling committee.c: too many arguments to function