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

[Markus Armbruster]

"Dr. David Alan Gilbert" <address@hidden> writes:

> * Markus Armbruster (address@hidden) wrote:
>> "Dr. David Alan Gilbert" <address@hidden> writes:
>> 
>> > * Markus Armbruster (address@hidden) wrote:
>> >> "Dr. David Alan Gilbert" <address@hidden> writes:
>> >> 
>> >> > * Markus Armbruster (address@hidden) wrote:
>> >> >> "Dr. David Alan Gilbert (git)" <address@hidden> writes:
>> >> >> 
>> >> >> > From: "Dr. David Alan Gilbert" <address@hidden>
>> >> >> >
>> >> >> > 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 <address@hidden>
>> >> >> >
>> >> >> > Updated for changed interface:
>> >> >> > Signed-off-by: Dr. David Alan Gilbert <address@hidden>
>> >> >> > ---
>> >> >> >  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 :)
>
> Yes, although I hate to turn a patchset for a tiny feature into a
> fix-all-the-broken-stuff set!

I know the feeling...

I'd love to accommodate you, but I'm afraid the work is too incomplete
in its current state.  The string output visitor doesn't implement a
number of things besides structs.  To convince me that your qom-get
won't crash because of that, you'd have to show that these other things
cannot happen with qom-get.  Implementing the missing parts instead
would probably be easier.  And then one of the general solutions
discussed below would hardly be more work, for more value.

>> 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.
>
> That's my preferred way of doing it, chaining constructors.

It's a fine way of doing it when you only ever create the things in one
way.  Here, with g_malloc0().

Would you like to wait for the Dan's visitor work?  Perhaps the problem
goes away there...

> Dave
>
>> (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).
> --
> Dr. David Alan Gilbert / address@hidden / Manchester, UK