[Qemu-devel] Re: [PATCH v3 2/3] qerror: Add a new MACHINE_STOPPED error message

[Daniel P. Berrange]

On Fri, Aug 27, 2010 at 07:39:37AM -0500, Anthony Liguori wrote:
> On 08/27/2010 04:29 AM, Daniel P. Berrange wrote:
> >On Fri, Aug 27, 2010 at 10:57:10AM +0530, Amit Shah wrote:
> >   
> >>This error message denotes some command was not successful in completing
> >>as the guest was unresponsive.
> >>
> >>Use it in the virtio-balloon code when showing older, cached data.
> >>
> >>Signed-off-by: Amit Shah<address@hidden>
> >>---
> >>  hw/virtio-balloon.c |    1 +
> >>  qerror.c            |    4 ++++
> >>  qerror.h            |    3 +++
> >>  3 files changed, 8 insertions(+), 0 deletions(-)
> >>
> >>diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
> >>index d6c66cf..309c343 100644
> >>--- a/hw/virtio-balloon.c
> >>+++ b/hw/virtio-balloon.c
> >>@@ -140,6 +140,7 @@ static void complete_stats_request(VirtIOBalloon *vb)
> >>
> >>  static void show_old_stats(void *opaque)
> >>  {
> >>+    qerror_report(QERR_MACHINE_STOPPED);
> >>      complete_stats_request(opaque);
> >>  }
> >>     
> >
> >NACK. It has always been allowed&  valid to call query-balloon
> >to get the current balloon level. We must not throw an error
> >just because the recently added mem stats can't be refreshed.
> 
> I think that's a fair comment but why even bother fixing the command.  
> Let's introduce a new command that just gets a single piece of 
> information instead of having a command return lots of information.

The existing query-balloon command that has been around for years &
is used by all current apps has a significant regression since we added
the memstats code to it: a guest can now trivially inflict a DOS on the
mgmt app if it crashes or is malicious. IMHO we need to fix that regression
for 0.13 so that existing apps don't suffer[1]. Adding a timeout to silently
skip the stats refresh if the guest doesn't respond, but without raising
an error seems the best tradeoff we can do here.

Beyond fixing that regression, I agree that this command is terminally
flawed & we need to deprecate it & provide better specified new
replacement(s). This seems like 0.14 work to me though.

Regards,
Daniel

[1] I know that they could already suffer if there was a bug in qemu
    that prevented it responding, even if the guest was not being
    malicious/crashed.
-- 
|: Red Hat, Engineering, London    -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org        -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|