savevm: print migration failure to stderr rather than monitor

Submitted by Alex Williamson on Jan. 7, 2011, 7:18 a.m.

Details

Message ID 20110107071815.26658.403.stgit@s20.home
State New
Headers show

Commit Message

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

Comments

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 hide | download patch | download mbox

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