Re: [Qemu-devel] [PATCH 1/2] qapi: Stub out StringOutputVisitor struct support

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Markus Armbruster (armbru@redhat.com) wrote:
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
> 
> > * Markus Armbruster (armbru@redhat.com) wrote:
> >> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:
> >> 
> >> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >> >
> >> > Avoid a segfault when visiting, e.g., the QOM rtc-time property,
> >> > by implementing the struct callbacks and raising an Error.
> >> >
> >> > Signed-off-by: Andreas Färber <afaerber@suse.de>
> >> >
> >> > Updated for changed interface:
> >> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> >> > ---
> >> >  qapi/string-output-visitor.c | 13 +++++++++++++
> >> >  1 file changed, 13 insertions(+)
> >> >
> >> > diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c
> >> > index 94ac821..4e7e97f 100644
> >> > --- a/qapi/string-output-visitor.c
> >> > +++ b/qapi/string-output-visitor.c
> >> > @@ -12,6 +12,7 @@
> >> >  
> >> >  #include "qemu/osdep.h"
> >> >  #include "qemu-common.h"
> >> > +#include "qapi/error.h"
> >> >  #include "qapi/string-output-visitor.h"
> >> >  #include "qapi/visitor-impl.h"
> >> >  #include "qemu/host-utils.h"
> >> > @@ -266,6 +267,16 @@ static void print_type_number(Visitor *v, const char *name, double *obj,
> >> >      string_output_set(sov, g_strdup_printf("%f", *obj));
> >> >  }
> >> >  
> >> > +static void start_struct(Visitor *v, const char *name, void **obj, size_t size,
> >> > +           Error **errp)
> >> > +{
> >> > +    error_setg(errp, "struct type not implemented");
> >> > +}
> >> > +
> >> > +static void end_struct(Visitor *v, void **obj)
> >> > +{
> >> > +}
> >> > +
> >> 
> >> This is just one of the several things this visitor doesn't implement.
> >> See the comment in string-output-visitor.h.
> >> 
> >> String input visitor and options visitor have similar holes; see the
> >> comments in string-input-visitor.h and opts-visitor.h.
> >> 
> >> Should we change all of them together to report errors instead of crash?
> >> With common "error out because this isn't implemented" methods?
> >
> > In that case wouldn't it be best to change visit_start_struct/visit_end_struct
> > to do the check (Like visit_check_struct does).
> 
> In my opinion.
> 
>     if (v->foo) {
>         v->foo(...);
>     } else {
>         ... default action ...
>     }
> 
> is an anti-pattern.  Wrap the default action in a default method, and
> put that in the function pointer.

I've got some sympathy to that, but with the way our visitors are
built that's a pain.

Lets say you add a new eat_struct method, and a eat_struct_default implementation,
now you have to go around and fix all the visitor implementations to initialise
their eat_struct member to eat_struct_default.   Of course you'll forget some
and then we'll end up segging when you fall down the NULL pointer.

Now, if our visitors had nice shared constructor functions that wouldn't
be a problem, and you wouldn't need most of the visit_ wrapper functions;
but they don't, so the if (v->foo) { ... } else { error; }   is the
current cleanest we can do.

Dave
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK