Re: [Qemu-devel] [PATCH v1 3/6] qapi: assert that visitor impls have required callbacks

[Daniel P. Berrange]

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 <address@hidden>
> > ---
> >  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 :|