[1/2] qapi: Stub out StringOutputVisitor struct support

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(+)

"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?

>  static void
>  start_list(Visitor *v, const char *name, GenericList **list, size_t size,
>             Error **errp)
> @@ -356,6 +367,8 @@ Visitor *string_output_visitor_new(bool human, char **result)
>      v->visitor.end_list = end_list;
>      v->visitor.complete = string_output_complete;
>      v->visitor.free = string_output_free;
> +    v->visitor.start_struct = start_struct;
> +    v->visitor.end_struct = end_struct;
>  
>      return &v->visitor;
>  }

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

Dave

> 
> >  static void
> >  start_list(Visitor *v, const char *name, GenericList **list, size_t size,
> >             Error **errp)
> > @@ -356,6 +367,8 @@ Visitor *string_output_visitor_new(bool human, char **result)
> >      v->visitor.end_list = end_list;
> >      v->visitor.complete = string_output_complete;
> >      v->visitor.free = string_output_free;
> > +    v->visitor.start_struct = start_struct;
> > +    v->visitor.end_struct = end_struct;
> >  
> >      return &v->visitor;
> >  }
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

* 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

"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).

* Markus Armbruster (armbru@redhat.com) wrote:
> "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 :)

Yes, although I hate to turn a patchset for a tiny feature into a
fix-all-the-broken-stuff set!

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

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 / dgilbert@redhat.com / Manchester, UK

"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" <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 :)
>
> 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 / dgilbert@redhat.com / Manchester, UK

* Markus Armbruster (armbru@redhat.com) wrote:
> "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" <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 :)
> >
> > 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...

Will it ?

Dave

> > 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 / dgilbert@redhat.com / Manchester, UK
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK