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

[Markus Armbruster <armbru@redhat.com>]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:

> * 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.

Well, it's the cleanest we can do as long as we constrain ourselves not
to do much :)

We currently have seven visitors.  Every single one defines a
FOO_visitor_new() function that basically looks like this:

    Visitor *FOO_visitor_new(... whatever ...)
    {
        FOOVisitor v = g_malloc0(sizeof(*v));

        v->visitor.type = ...
        ... initialize more of v->visitor ...
        ... initialize other members of *v, if any ...

        return &v->visitor;
    }

I grant you that putting sensible defaults into v->visitor by
initializing them correctly in all the FOO_visitor_new() functions is a
bit of pain.  Not much pain; there are only seven.  Anyway, there are
several obvious ways to do this without pain:

(1) Have a visitor core function to set the defaults, call it first.

(2) Replace g_malloc0() by a visitor core function that additionally
    sets the defaults.  Basically fusing g_malloc0() into (1)'s
    function.

(3) Have a visitor core function that replaces null methods by defaults,
    and call it last.  This function can also check you filled out in
    the mandatory bits.  Have it return the visitor, so you can make it
    a tail call: return visitor_check(&v->visitor).