| Message ID | 20110107071815.26658.403.stgit@s20.home |
|---|---|
| State | New |
| Headers | show |
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; > } > } > > >
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; > > } > > } > > > > > > >
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
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
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; } }
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(-)