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