From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34599) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1axt1Z-0007Ak-KX for qemu-devel@nongnu.org; Wed, 04 May 2016 05:23:24 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1axt1N-0000t2-Ja for qemu-devel@nongnu.org; Wed, 04 May 2016 05:23:12 -0400 Received: from mx1.redhat.com ([209.132.183.28]:37134) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1axt1N-0000qj-CC for qemu-devel@nongnu.org; Wed, 04 May 2016 05:23:05 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id B97953DD47 for ; Wed, 4 May 2016 09:22:53 +0000 (UTC) Date: Wed, 4 May 2016 10:22:49 +0100 From: "Dr. David Alan Gilbert" Message-ID: <20160504092249.GC2302@work-vm> References: <1461903820-3092-1-git-send-email-eblake@redhat.com> <1461903820-3092-11-git-send-email-eblake@redhat.com> <20160503094447.GE2242@work-vm> <87inyv5nv7.fsf@dusky.pond.sub.org> <20160503132358.GH2242@work-vm> <87eg9iyypp.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87eg9iyypp.fsf@dusky.pond.sub.org> Subject: Re: [Qemu-devel] [PATCH v3 10/18] vmstate: Use new JSON output visitor List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: Amit Shah , famz@redhat.com, qemu-devel@nongnu.org, Juan Quintela * Markus Armbruster (armbru@redhat.com) wrote: > "Dr. David Alan Gilbert" writes: > "git-grep assert migration" suggests you do kill the source on certain > programming errors. I'm just trying hard to reduce them; I know I'm not there, but I'd rather we didn't have any - especially on the source side. > I reiterate my point that fancy, untestable error recovery is unlikely > to actually recover. "Fancy" can work, "untestable" might work (but > color me skeptic), but once you got both, you're a dead man walking. Then we should make the error recovery paths easy; at the moment visitor error paths are just too painful. > > >> Complete list of conditions where the JSON output visitor sets an error: > >> > >> * Conditions where the visitor core sets an error: > >> > >> - visit_type_uintN() when one of the visit_type_uint{8,16,32}() passes > >> a value out of bounds. This is a serious programming error in > >> qapi-visit-core.c. We're almost certainly screwed, and attempting > >> to continue is unsafe. > >> > >> - visit_type_int(): likewise. > >> > >> - output_type_enum() when the numeric value is out of bounds. This is > >> either a serious programming error in qapi-visit-core.c, or > >> corrupted state. Either way, we're almost certainly screwed, and > >> attempting to continue is unsafe. > >> > >> - input_type_enum() when the string value is unknown. This is either > >> a serious programming error in qapi-visit-core.c, or bad input. > >> However, the JSON output visitor isn't supposed to ever call > >> input_type_enum(), so it's the former. Once again, we're almost > >> certainly screwed, and attempting to continue is unsafe. > >> > >> * Conditions where the JSON output visitor itself sets an error: > >> > >> - None. > >> > >> Do you still object to &error_abort? > > > > So at the very least it should be commented as to why it can't happen. > > My worry about it is that you've got a fairly long comment about why > > it can't happen, and I worry that in 6 months someone adds a feature > > to either the visitors or the migration code that means there's now > > a case where it can happen. > > Here's why I don't think new failure modes are likely. > > What does this helper module do, and how could it possibly fail? By > "possibly", I mean any conceivable reasonable implementation, not just > the two we have (this patch gets rid of one). > > This helper module builds JSON text and returns it as a string. Its > interface mirrors JSON abstract syntax: start object, end object, start > array, end array, string, ... Additionally, initialize, finalize, get > the result as a string. > > Conceivable failure modes: > > * Out of memory. We die, like we generally do for smallish allocations. > > * Data not representable in JSON. This is basically non-finite numbers, > and we already chose to extend JSON instead of making this an error. > Such a decision will not be revised without a thorough analysis of > impact on existing users. > > * Interface misused, e.g. invalid nesting. Clearly a programming error. > We can either silently produce garbage output, fail, or die. Before > the patch: garbage output. After the patch: die by assertion failure > (*not* via &error_abort). > > * Anything else? > > "Not via &error_abort" leads me to another point. The &error_abort are > the assertions you can see in the patch. The ones you can't see are in > the visitor core and the JSON output visitor. They're all about misuse > of the interface. > > The old code is different: it doesn't detect misuse, and produces > invalid JSON instead. "Never check for an error you don't know how to > handle." > > With the new code, misuse should be caught in general migration testing, > "make check" if it's any good. > > With the old code, it could more easily escape testing, because you have > to parse the resulting JSON to detect it. And what happens to the users VM if that JSON is invalid? *nothing* The user doesn't see any problem at all; no corruption, no crash, nothing. That's what I like users to see. Dave -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK