[Qemu-devel] [PATCH] savevm: print migration failure to stderr rather than monitor

[Qemu-devel] [PATCH] savevm: print migration failure to stderr rather than monitor

[Alex Williamson]

monitor_print only does anything for foreground commands, so we
don't ever see this error message in the case of a 'migrate -d'.
It also doesn't do much good to print a monitor error message if
the migration is being driven by something like libvirt.  Both
of these seem to be the typical usage scenarios, so we might as
well print this error to stderr so it can at least be found in
the log messages.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---

 savevm.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/savevm.c b/savevm.c
index 90aa237..c6b9b01 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1543,7 +1543,7 @@ int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f)
 
         r = vmstate_save(f, se);
         if (r < 0) {
-            monitor_printf(mon, "cannot migrate with device '%s'\n", se->idstr);
+            fprintf(stderr, "cannot migrate with device '%s'\n", se->idstr);
             return r;
         }
     }

[Qemu-devel] Re: [PATCH] savevm: print migration failure to stderr rather than monitor

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1452 bytes --]

Am 07.01.2011 08:18, Alex Williamson wrote:
> monitor_print only does anything for foreground commands, so we
> don't ever see this error message in the case of a 'migrate -d'.

Your change needlessly steals the error from the monitor console where
it belongs if migrate is used without -d. IIRC, mon is NULL in detached
mode, so only print to stderr if there is no alternative. Otherwise
stick with the monitor for interactive use.

> It also doesn't do much good to print a monitor error message if
> the migration is being driven by something like libvirt.  Both

There is not only libvirt. Please don't destroy HMP "experience".

Jan

> of these seem to be the typical usage scenarios, so we might as
> well print this error to stderr so it can at least be found in
> the log messages.
> 
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
> 
>  savevm.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/savevm.c b/savevm.c
> index 90aa237..c6b9b01 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -1543,7 +1543,7 @@ int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f)
>  
>          r = vmstate_save(f, se);
>          if (r < 0) {
> -            monitor_printf(mon, "cannot migrate with device '%s'\n", se->idstr);
> +            fprintf(stderr, "cannot migrate with device '%s'\n", se->idstr);
>              return r;
>          }
>      }
> 
> 
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

[Qemu-devel] Re: [PATCH] savevm: print migration failure to stderr rather than monitor

[Alex Williamson]

On Fri, 2011-01-07 at 09:51 +0100, Jan Kiszka wrote:
> Am 07.01.2011 08:18, Alex Williamson wrote:
> > monitor_print only does anything for foreground commands, so we
> > don't ever see this error message in the case of a 'migrate -d'.
> 
> Your change needlessly steals the error from the monitor console where
> it belongs if migrate is used without -d. IIRC, mon is NULL in detached
> mode, so only print to stderr if there is no alternative. Otherwise
> stick with the monitor for interactive use.

Indeed, mon is NULL.  That makes this an easy

if (mon) {
    monitor_printf()
} else {
    fprintf()
}

But I wonder if we should put the fprintf in the monitor_printf() path
so we're not just special casing this one user.  Should all
monitor_printfs go to stderr if there's no monitor?  Thanks,

Alex

> > It also doesn't do much good to print a monitor error message if
> > the migration is being driven by something like libvirt.  Both
> 
> There is not only libvirt. Please don't destroy HMP "experience".
> 
> Jan
> 
> > of these seem to be the typical usage scenarios, so we might as
> > well print this error to stderr so it can at least be found in
> > the log messages.
> > 
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > ---
> > 
> >  savevm.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/savevm.c b/savevm.c
> > index 90aa237..c6b9b01 100644
> > --- a/savevm.c
> > +++ b/savevm.c
> > @@ -1543,7 +1543,7 @@ int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f)
> >  
> >          r = vmstate_save(f, se);
> >          if (r < 0) {
> > -            monitor_printf(mon, "cannot migrate with device '%s'\n", se->idstr);
> > +            fprintf(stderr, "cannot migrate with device '%s'\n", se->idstr);
> >              return r;
> >          }
> >      }
> > 
> > 
> > 
> 

[Qemu-devel] Re: [PATCH] savevm: print migration failure to stderr rather than monitor

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1126 bytes --]

Am 07.01.2011 16:39, Alex Williamson wrote:
> On Fri, 2011-01-07 at 09:51 +0100, Jan Kiszka wrote:
>> Am 07.01.2011 08:18, Alex Williamson wrote:
>>> monitor_print only does anything for foreground commands, so we
>>> don't ever see this error message in the case of a 'migrate -d'.
>>
>> Your change needlessly steals the error from the monitor console where
>> it belongs if migrate is used without -d. IIRC, mon is NULL in detached
>> mode, so only print to stderr if there is no alternative. Otherwise
>> stick with the monitor for interactive use.
> 
> Indeed, mon is NULL.  That makes this an easy
> 
> if (mon) {
>     monitor_printf()
> } else {
>     fprintf()
> }
> 
> But I wonder if we should put the fprintf in the monitor_printf() path
> so we're not just special casing this one user.  Should all
> monitor_printfs go to stderr if there's no monitor?  Thanks,

IIRC, there are valid cased where you want to suppress status updates of
some subsystem by handing out a NULL monitor.

If this error is critical (likely), then user error_report instead. It
does the right thing.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

[Qemu-devel] Re: [PATCH] savevm: print migration failure to stderr rather than monitor

[Alex Williamson]

On Fri, 2011-01-07 at 16:46 +0100, Jan Kiszka wrote:
> Am 07.01.2011 16:39, Alex Williamson wrote:
> > On Fri, 2011-01-07 at 09:51 +0100, Jan Kiszka wrote:
> >> Am 07.01.2011 08:18, Alex Williamson wrote:
> >>> monitor_print only does anything for foreground commands, so we
> >>> don't ever see this error message in the case of a 'migrate -d'.
> >>
> >> Your change needlessly steals the error from the monitor console where
> >> it belongs if migrate is used without -d. IIRC, mon is NULL in detached
> >> mode, so only print to stderr if there is no alternative. Otherwise
> >> stick with the monitor for interactive use.
> > 
> > Indeed, mon is NULL.  That makes this an easy
> > 
> > if (mon) {
> >     monitor_printf()
> > } else {
> >     fprintf()
> > }
> > 
> > But I wonder if we should put the fprintf in the monitor_printf() path
> > so we're not just special casing this one user.  Should all
> > monitor_printfs go to stderr if there's no monitor?  Thanks,
> 
> IIRC, there are valid cased where you want to suppress status updates of
> some subsystem by handing out a NULL monitor.
> 
> If this error is critical (likely), then user error_report instead. It
> does the right thing.

Thanks, that works well.  Follow-up to come.

Alex

[Qemu-devel] [PATCH V2] savevm: use error_report for vmstate_save error

[Alex Williamson]

If migration is done in the background with the -d option, mon is
NULL and this error message is lost.  Instead use error_report().

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---

 savevm.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/savevm.c b/savevm.c
index 90aa237..148871d 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1543,7 +1543,7 @@ int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f)
 
         r = vmstate_save(f, se);
         if (r < 0) {
-            monitor_printf(mon, "cannot migrate with device '%s'\n", se->idstr);
+            error_report("cannot migrate with device '%s'\n", se->idstr);
             return r;
         }
     }

[Qemu-devel] Re: [PATCH V2] savevm: use error_report for vmstate_save error

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 968 bytes --]

Am 07.01.2011 16:58, Alex Williamson wrote:
> If migration is done in the background with the -d option, mon is
> NULL and this error message is lost.  Instead use error_report().
> 
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>

Reviewed-by: Jan Kiszka <jan.kiszka@siemens.com>

As already at it: Is this the only error during migration that can get
lost this way?

Jan

> ---
> 
>  savevm.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/savevm.c b/savevm.c
> index 90aa237..148871d 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -1543,7 +1543,7 @@ int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f)
>  
>          r = vmstate_save(f, se);
>          if (r < 0) {
> -            monitor_printf(mon, "cannot migrate with device '%s'\n", se->idstr);
> +            error_report("cannot migrate with device '%s'\n", se->idstr);
>              return r;
>          }
>      }
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

[Qemu-devel] Re: [PATCH V2] savevm: use error_report for vmstate_save error

[Alex Williamson]

On Fri, 2011-01-07 at 17:03 +0100, Jan Kiszka wrote:
> Am 07.01.2011 16:58, Alex Williamson wrote:
> > If migration is done in the background with the -d option, mon is
> > NULL and this error message is lost.  Instead use error_report().
> > 
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> 
> Reviewed-by: Jan Kiszka <jan.kiszka@siemens.com>
> 
> As already at it: Is this the only error during migration that can get
> lost this way?

On the migration source side, I think so.  All the save callbacks are
return void, so device saves aren't allow to fail.  The unmigratable
device registration is the only way I know to trigger an error on this
end.  Thanks,

Alex

> > ---
> > 
> >  savevm.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/savevm.c b/savevm.c
> > index 90aa237..148871d 100644
> > --- a/savevm.c
> > +++ b/savevm.c
> > @@ -1543,7 +1543,7 @@ int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f)
> >  
> >          r = vmstate_save(f, se);
> >          if (r < 0) {
> > -            monitor_printf(mon, "cannot migrate with device '%s'\n", se->idstr);
> > +            error_report("cannot migrate with device '%s'\n", se->idstr);
> >              return r;
> >          }
> >      }
> > 
> 
> 

[Qemu-devel] Re: [PATCH V2] savevm: use error_report for vmstate_save error

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 924 bytes --]

Am 07.01.2011 17:10, Alex Williamson wrote:
> On Fri, 2011-01-07 at 17:03 +0100, Jan Kiszka wrote:
>> Am 07.01.2011 16:58, Alex Williamson wrote:
>>> If migration is done in the background with the -d option, mon is
>>> NULL and this error message is lost.  Instead use error_report().
>>>
>>> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
>>
>> Reviewed-by: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> As already at it: Is this the only error during migration that can get
>> lost this way?
> 
> On the migration source side, I think so.  All the save callbacks are
> return void, so device saves aren't allow to fail.  The unmigratable
> device registration is the only way I know to trigger an error on this
> end.  Thanks,

Hmm, thinking about this again, wouldn't it be a better user experience
to check this in advance, at a time where the issuing console is still
listening?

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

[Qemu-devel] [PATCH v3] savevm: Fix no_migrate

[Alex Williamson]

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

[Qemu-devel] Re: [PATCH V2] savevm: use error_report for vmstate_save error

[Alex Williamson]

On Fri, 2011-01-07 at 17:27 +0100, Jan Kiszka wrote:
> Am 07.01.2011 17:10, Alex Williamson wrote:
> > On Fri, 2011-01-07 at 17:03 +0100, Jan Kiszka wrote:
> >> Am 07.01.2011 16:58, Alex Williamson wrote:
> >>> If migration is done in the background with the -d option, mon is
> >>> NULL and this error message is lost.  Instead use error_report().
> >>>
> >>> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> >>
> >> Reviewed-by: Jan Kiszka <jan.kiszka@siemens.com>
> >>
> >> As already at it: Is this the only error during migration that can get
> >> lost this way?
> > 
> > On the migration source side, I think so.  All the save callbacks are
> > return void, so device saves aren't allow to fail.  The unmigratable
> > device registration is the only way I know to trigger an error on this
> > end.  Thanks,
> 
> Hmm, thinking about this again, wouldn't it be a better user experience
> to check this in advance, at a time where the issuing console is still
> listening?

I hope v3 addresses this as you were hoping.  It also fixes a couple
other issues with no_migrate.  Thanks,

Alex

[Qemu-devel] Re: [PATCH v3] savevm: Fix no_migrate

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 5159 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

[Qemu-devel] [PATCH v4] savevm: Fix no_migrate

[Alex Williamson]

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

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

[Qemu-devel] Re: [PATCH v3] savevm: Fix no_migrate

[Michael S. Tsirkin]

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/

-- 
MST

[Qemu-devel] Re: [PATCH v4] savevm: Fix no_migrate

[Michael S. Tsirkin]

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

[Qemu-devel] Re: [PATCH v4] savevm: Fix no_migrate

[Michael S. Tsirkin]

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

Re: [Qemu-devel] Re: [PATCH v3] savevm: Fix no_migrate

[Blue Swirl]

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.

[Qemu-devel] Re: [PATCH v4] savevm: Fix no_migrate

[Daniel P. Berrange]

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

[Qemu-devel] Re: [PATCH v4] savevm: Fix no_migrate

[Alex Williamson]

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

[Qemu-devel] Re: [PATCH v4] savevm: Fix no_migrate

[Alex Williamson]

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

[Qemu-devel] Re: [PATCH v4] savevm: Fix no_migrate

[Michael S. Tsirkin]

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

[Qemu-devel] [PATCH v5] savevm: Fix no_migrate

[Alex Williamson]

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

v5:
 - change qemu_savevm_state_blocked() to return bool
 - Daniel confirmed libvirt will not have a problem with the behavior
   change on the target end.

 migration.c |    4 ++++
 savevm.c    |   40 ++++++++++++++++++++++++++--------------
 sysemu.h    |    1 +
 3 files changed, 31 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..fcd8db4 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
 
+bool 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 true;
+        }
+    }
+    return false;
+}
+
 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);
 
+    if (qemu_savevm_state_blocked(mon)) {
+        ret = -EINVAL;
+        goto out;
+    }
+
     ret = qemu_savevm_state_begin(mon, f, 0, 0);
     if (ret < 0)
         goto out;
@@ -1692,6 +1700,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 d8fceec..0c969f2 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -75,6 +75,7 @@ void qemu_announce_self(void);
 
 void main_loop_wait(int nonblocking);
 
+bool 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);

[Qemu-devel] Re: [PATCH v5] savevm: Fix no_migrate

[Michael S. Tsirkin]

On Tue, Jan 11, 2011 at 02:39:43PM -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>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> ---
> 
> v5:
>  - change qemu_savevm_state_blocked() to return bool
>  - Daniel confirmed libvirt will not have a problem with the behavior
>    change on the target end.
> 
>  migration.c |    4 ++++
>  savevm.c    |   40 ++++++++++++++++++++++++++--------------
>  sysemu.h    |    1 +
>  3 files changed, 31 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..fcd8db4 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
>  
> +bool 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 true;
> +        }
> +    }
> +    return false;
> +}
> +
>  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);
>  
> +    if (qemu_savevm_state_blocked(mon)) {
> +        ret = -EINVAL;
> +        goto out;
> +    }
> +
>      ret = qemu_savevm_state_begin(mon, f, 0, 0);
>      if (ret < 0)
>          goto out;
> @@ -1692,6 +1700,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 d8fceec..0c969f2 100644
> --- a/sysemu.h
> +++ b/sysemu.h
> @@ -75,6 +75,7 @@ void qemu_announce_self(void);
>  
>  void main_loop_wait(int nonblocking);
>  
> +bool 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);