On Mon, Jan 10, 2011 at 10:47:58AM -0700, Alex Williamson wrote: > On Sun, 2011-01-09 at 12:47 +0200, Michael S. Tsirkin wrote: > > On Fri, Jan 07, 2011 at 03:13:25PM -0700, Alex Williamson wrote: > > > The no_migrate save state flag is currently only checked in the > > > last phase of migration. This means that we potentially waste > > > a lot of time and bandwidth with the live state handlers before > > > we ever check the no_migrate flags. The error message printed > > > when we catch a non-migratable device doesn't get printed for > > > a detached migration. And, no_migrate does nothing to prevent > > > an incoming migration to a target that includes a non-migratable > > > device. This attempts to fix all of these. > > > > > > One notable difference in behavior is that an outgoing migration > > > now checks for non-migratable devices before ever connecting to > > > the target system. This means the target will remain listening > > > rather than exit from failure. > > > > > > Signed-off-by: Alex Williamson <alex.william...@redhat.com> > > > > Minor nit: > > > > The API of qemu_savevm_state_blocked is a bit strange. > > functions should either return 0/1 values or 0 on success > > negative on failure or a bool. > > This API asks "is it blocked" and returns -EINVAL to > > mean "yes". _blocked is also a bit ambigious: > > it seems to imply a temporary condition. > > But it very well could be a temporary condition. If I have an assigned > device, it will block migration/savevm, but removing the device allows > it to proceed.
Well to me blocked it means migration will not fail, but will be blocked and unblocked later when device is removed. > > How about we reverse the logic, call the new API > > qemu_savevm_state_supported, qemu_savevm_state_enabled, > > something like this? > > Then you can return 0 if migration is possible, > > -1 if not. > > I tend to like qemu_savevm_state_blocked() as "supported" and "enabled" > invoke different ideas for me. I will concede that the errno return > value is a little awkward. How about if I make it a bool function > instead? Thanks, > > Alex That's better. > > > --- > > > > > > v4: > > > - fix braces noted by Jan > > > - return error from qemu_savevm_state_blocked rather than fixed EINVAL > > > at qemu_loadvm_state(), since it'a already using errno values > > > > > > v3: > > > > > > Daniel, adding you to see if libvirt cares about the difference in > > > whether the target exits on migration failure as noted above. > > > > > > Also adding Michael as he's complained about the no_migrate check > > > happening so late in the process. > > > > > > migration.c | 4 ++++ > > > savevm.c | 41 +++++++++++++++++++++++++++-------------- > > > sysemu.h | 1 + > > > 3 files changed, 32 insertions(+), 14 deletions(-) > > > > > > diff --git a/migration.c b/migration.c > > > index e5ba51c..d593b1d 100644 > > > --- a/migration.c > > > +++ b/migration.c > > > @@ -88,6 +88,10 @@ int do_migrate(Monitor *mon, const QDict *qdict, > > > QObject **ret_data) > > > return -1; > > > } > > > > > > + if (qemu_savevm_state_blocked(mon)) { > > > + return -1; > > > + } > > > + > > > if (strstart(uri, "tcp:", &p)) { > > > s = tcp_start_outgoing_migration(mon, p, max_throttle, detach, > > > blk, inc); > > > diff --git a/savevm.c b/savevm.c > > > index 90aa237..34c0d1a 100644 > > > --- a/savevm.c > > > +++ b/savevm.c > > > @@ -1401,19 +1401,13 @@ static int vmstate_load(QEMUFile *f, > > > SaveStateEntry *se, int version_id) > > > return vmstate_load_state(f, se->vmsd, se->opaque, version_id); > > > } > > > > > > -static int vmstate_save(QEMUFile *f, SaveStateEntry *se) > > > +static void vmstate_save(QEMUFile *f, SaveStateEntry *se) > > > { > > > - if (se->no_migrate) { > > > - return -1; > > > - } > > > - > > > if (!se->vmsd) { /* Old style */ > > > se->save_state(f, se->opaque); > > > - return 0; > > > + return; > > > } > > > vmstate_save_state(f,se->vmsd, se->opaque); > > > - > > > - return 0; > > > } > > > > > > #define QEMU_VM_FILE_MAGIC 0x5145564d > > > @@ -1427,6 +1421,20 @@ static int vmstate_save(QEMUFile *f, > > > SaveStateEntry *se) > > > #define QEMU_VM_SECTION_FULL 0x04 > > > #define QEMU_VM_SUBSECTION 0x05 > > > > > > +int qemu_savevm_state_blocked(Monitor *mon) > > > +{ > > > + SaveStateEntry *se; > > > + > > > + QTAILQ_FOREACH(se, &savevm_handlers, entry) { > > > + if (se->no_migrate) { > > > + monitor_printf(mon, "state blocked by non-migratable device > > > '%s'\n", > > > + se->idstr); > > > + return -EINVAL; > > > + } > > > + } > > > + return 0; > > > +} > > > + > > > int qemu_savevm_state_begin(Monitor *mon, QEMUFile *f, int blk_enable, > > > int shared) > > > { > > > @@ -1508,7 +1516,6 @@ int qemu_savevm_state_iterate(Monitor *mon, > > > QEMUFile *f) > > > int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f) > > > { > > > SaveStateEntry *se; > > > - int r; > > > > > > cpu_synchronize_all_states(); > > > > > > @@ -1541,11 +1548,7 @@ int qemu_savevm_state_complete(Monitor *mon, > > > QEMUFile *f) > > > qemu_put_be32(f, se->instance_id); > > > qemu_put_be32(f, se->version_id); > > > > > > - r = vmstate_save(f, se); > > > - if (r < 0) { > > > - monitor_printf(mon, "cannot migrate with device '%s'\n", > > > se->idstr); > > > - return r; > > > - } > > > + vmstate_save(f, se); > > > } > > > > > > qemu_put_byte(f, QEMU_VM_EOF); > > > @@ -1575,6 +1578,11 @@ static int qemu_savevm_state(Monitor *mon, > > > QEMUFile *f) > > > saved_vm_running = vm_running; > > > vm_stop(0); > > > > > > + ret = qemu_savevm_state_blocked(mon); > > > + if (ret < 0) { > > > + goto out; > > > + } > > > + > > > ret = qemu_savevm_state_begin(mon, f, 0, 0); > > > if (ret < 0) > > > goto out; > > > @@ -1692,6 +1700,11 @@ int qemu_loadvm_state(QEMUFile *f) > > > unsigned int v; > > > int ret; > > > > > > + ret = qemu_savevm_state_blocked(default_mon); > > > + if (ret < 0) { > > > + return ret; > > > + } > > > + > > > v = qemu_get_be32(f); > > > if (v != QEMU_VM_FILE_MAGIC) > > > return -EINVAL; > > > diff --git a/sysemu.h b/sysemu.h > > > index e728ea1..eefaba5 100644 > > > --- a/sysemu.h > > > +++ b/sysemu.h > > > @@ -75,6 +75,7 @@ void qemu_announce_self(void); > > > > > > void main_loop_wait(int nonblocking); > > > > > > +int qemu_savevm_state_blocked(Monitor *mon); > > > int qemu_savevm_state_begin(Monitor *mon, QEMUFile *f, int blk_enable, > > > int shared); > > > int qemu_savevm_state_iterate(Monitor *mon, QEMUFile *f); > >