"Daniel P. Berrange" <berra...@redhat.com> writes: > 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<amit.s...@redhat.com> >> >>--- >> >> 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.
I agree. Adding a roundtrip through the guest to an existing command was a mistake. > 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. Yup. > 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.