Patchwork [V2] savevm: use error_report for vmstate_save error

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

Comments

Alex Williamson - Jan. 7, 2011, 3:58 p.m.
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(-)
Jan Kiszka - Jan. 7, 2011, 4:03 p.m.
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;
>          }
>      }
>
Alex Williamson - Jan. 7, 2011, 4:10 p.m.
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;
> >          }
> >      }
> > 
> 
>
Jan Kiszka - Jan. 7, 2011, 4:27 p.m.
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
Alex Williamson - Jan. 7, 2011, 6:41 p.m.
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

Patch

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