Patchwork [v4] savevm: Fix no_migrate

login
register
mail settings
Submitter Alex Williamson
Date Jan. 7, 2011, 10:13 p.m.
Message ID <20110107220801.15395.38757.stgit@s20.home>
Download mbox | patch
Permalink /patch/77922/
State New
Headers show

Comments

Alex Williamson - Jan. 7, 2011, 10:13 p.m.
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.williamson@redhat.com>
---

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(-)
Michael S. Tsirkin - Jan. 9, 2011, 10:02 a.m.
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.

Nod. Sounds good.

> 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.williamson@redhat.com>

Looks like a feature, but it won't be hard to make it exit
if we want it for compatibility....

> ---
> 
> 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);
Michael S. Tsirkin - Jan. 9, 2011, 10:47 a.m.
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.williamson@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.

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.

> ---
> 
> 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);
Daniel P. Berrange - Jan. 10, 2011, 10:24 a.m.
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.williamson@redhat.com>
> ---
> 
> 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.

If the 'migrate' command on the source QEMU returns an error,
then libvirt will teardown the target QEMU automatically, so
that's not a problem.

Regards,
Daniel
Alex Williamson - Jan. 10, 2011, 2:52 p.m.
On Mon, 2011-01-10 at 10:24 +0000, Daniel P. Berrange 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.williamson@redhat.com>
> > ---
> > 
> > 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.
> 
> If the 'migrate' command on the source QEMU returns an error,
> then libvirt will teardown the target QEMU automatically, so
> that's not a problem.

Thanks, that's the way I was hoping it would work.

Alex
Alex Williamson - Jan. 10, 2011, 5:47 p.m.
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.williamson@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.

> 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

> > ---
> > 
> > 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);
Michael S. Tsirkin - Jan. 10, 2011, 9:19 p.m.
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.williamson@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);
> 
>

Patch

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);