Patchwork savevm: print migration failure to stderr rather than monitor

login
register
mail settings
Submitter Alex Williamson
Date Jan. 7, 2011, 7:18 a.m.
Message ID <20110107071815.26658.403.stgit@s20.home>
Download mbox | patch
Permalink /patch/77855/
State New
Headers show

Comments

Alex Williamson - Jan. 7, 2011, 7:18 a.m.
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(-)
Jan Kiszka - Jan. 7, 2011, 8:51 a.m.
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;
>          }
>      }
> 
> 
>
Alex Williamson - Jan. 7, 2011, 3:39 p.m.
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;
> >          }
> >      }
> > 
> > 
> > 
>
Jan Kiszka - Jan. 7, 2011, 3:46 p.m.
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
Alex Williamson - Jan. 7, 2011, 3:56 p.m.
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

Patch

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