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 <berra...@redhat.com> > > --- > > 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 :|
