From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=37213 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OoziG-0000h9-Pi for qemu-devel@nongnu.org; Fri, 27 Aug 2010 10:15:29 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OoziB-0005Lr-1I for qemu-devel@nongnu.org; Fri, 27 Aug 2010 10:15:24 -0400 Received: from mx1.redhat.com ([209.132.183.28]:58881) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OoziA-0005LT-KA for qemu-devel@nongnu.org; Fri, 27 Aug 2010 10:15:19 -0400 Date: Fri, 27 Aug 2010 11:15:07 -0300 From: Luiz Capitulino Subject: Re: [Qemu-devel] Re: [PATCH v3 2/3] qerror: Add a new MACHINE_STOPPED error message Message-ID: <20100827111507.5278eba3@doriath> In-Reply-To: References: <51ec99ce2db02aeb34ec6683a76895b4a127057d.1282886503.git.amit.shah@redhat.com> <20100827092945.GC22361@redhat.com> <4C77B209.6050902@codemonkey.ws> <20100827125827.GD22361@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: qemu list , agl@us.ibm.com, Amit Shah , Paolo Bonzini On Fri, 27 Aug 2010 15:59:21 +0200 Markus Armbruster wrote: > "Daniel P. Berrange" 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 > >> >>--- > >> >> 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. I wondered if we could drop it for now to make it right in 0.14, but I believe it's already part of the user monitor for some time and libvirt uses the stats, right? I think we need testing/unstable namespace in QMP, where commands can be tested for while so that we reduce the risk of nasty surprises like this one. > > > 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. >