From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48144) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bAJDe-00084Y-Ov for qemu-devel@nongnu.org; Tue, 07 Jun 2016 11:47:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bAJDc-0004Gk-DN for qemu-devel@nongnu.org; Tue, 07 Jun 2016 11:47:05 -0400 Date: Tue, 7 Jun 2016 16:46:53 +0100 From: "Daniel P. Berrange" Message-ID: <20160607154653.GQ20196@redhat.com> Reply-To: "Daniel P. Berrange" References: <1465294275-8733-1-git-send-email-berrange@redhat.com> <1465294275-8733-4-git-send-email-berrange@redhat.com> <5756EAE9.2040108@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <5756EAE9.2040108@redhat.com> Subject: Re: [Qemu-devel] [PATCH v1 3/6] qapi: assert that visitor impls have required callbacks List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: qemu-devel@nongnu.org, qemu-block@nongnu.org, Kevin Wolf , Max Reitz , Markus Armbruster , Michael Roth On Tue, Jun 07, 2016 at 09:40:25AM -0600, Eric Blake wrote: > On 06/07/2016 04:11 AM, Daniel P. Berrange wrote: > > Not all visitor implementations supply the full set of > > visitor callback functions. For example, the string > > output visitor does not provide 'start_struct' and > > friends. If you don't know this and feed it an object > > that uses structs, you'll get a crash: > > > > Segmentation fault (core dumped) > > > > Crashing is fine, because this is a programmer mistake, > > but we can improve the error message upon crash to make > > it obvious what failed by adding assert()s: > > > > qapi/qapi-visit-core.c:32: visit_start_struct: Assertion `v->start_struct != ((void *)0)' failed. > > > > Signed-off-by: Daniel P. Berrange > > --- > > qapi/qapi-visit-core.c | 15 +++++++++++++++ > > 1 file changed, 15 insertions(+) > > Up to Markus if he likes this (I think I've proposed the idea, but never > actually written it as a patch, because he implied that the core dump > still points people in the right direction). I think from the error message I show in the description above that the new crash is much more "user friendly" - if I saw a bug report with that assertion message I'd be more likely to dive in an fix it becasue it's obvious where the issue is, while a report with just a "Segmentation fault" could be anything. > > diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c > > index eada467..3b5efbe 100644 > > --- a/qapi/qapi-visit-core.c > > +++ b/qapi/qapi-visit-core.c > > @@ -29,6 +29,7 @@ void visit_start_struct(Visitor *v, const char *name, void **obj, > > assert(size); > > assert(v->type != VISITOR_OUTPUT || *obj); > > } > > + assert(v->start_struct != NULL); > > I'd have written it assert(v->start_struct) (explicit comparison against > NULL matters in Java, but is just line noise in C). The only reason I included the != NULL, is that it makes the corresponding assertion message printed a little more obvious eg Assertion `v->start_struct != ((void *)0)' failed. vs Assertion `v->start_struct' failed. Sure, seasoned C programmers will know they're the same, but the less experianced would likely find the former more obvious at first glance Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|