[v3] savevm: Fix no_migrate

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

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    |   39 +++++++++++++++++++++++++--------------
 sysemu.h    |    1 +
 3 files changed, 30 insertions(+), 14 deletions(-)

Am 07.01.2011 19:39, 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>
> ---
> 
> 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    |   39 +++++++++++++++++++++++++--------------
>  sysemu.h    |    1 +
>  3 files changed, 30 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..6675c33 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,10 @@ 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;
> +

{ } :)

(Is there really no one bored out there and wants to write a check-patch
script?)

>      ret = qemu_savevm_state_begin(mon, f, 0, 0);
>      if (ret < 0)
>          goto out;
> @@ -1692,6 +1699,10 @@ int qemu_loadvm_state(QEMUFile *f)
>      unsigned int v;
>      int ret;
>  
> +    if (qemu_savevm_state_blocked(default_mon)) {
> +        return -EINVAL;
> +    }
> +
>      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);
> 

Much nicer!

Jan

On Fri, Jan 07, 2011 at 07:47:34PM +0100, Jan Kiszka wrote:
> (Is there really no one bored out there and wants to write a check-patch
> script?)

Might be better to use an existing tool:
http://www.kernel.org/pub/linux/kernel/people/apw/checkpatch/

On Sun, Jan 9, 2011 at 9:57 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Fri, Jan 07, 2011 at 07:47:34PM +0100, Jan Kiszka wrote:
>> (Is there really no one bored out there and wants to write a check-patch
>> script?)
>
> Might be better to use an existing tool:
> http://www.kernel.org/pub/linux/kernel/people/apw/checkpatch/

Thanks for the link, I had previously tried to use something else.
I'll send patches to add checkpatch.pl for QEMU.