* Markus Armbruster (arm...@redhat.com) wrote:
> "Dr. David Alan Gilbert" <dgilb...@redhat.com> writes:
>
> > * Markus Armbruster (arm...@redhat.com) wrote:
> >> "Dr. David Alan Gilbert" <dgilb...@redhat.com> writes:
> >>
> >> > * Markus Armbruster (arm...@redhat.com) wrote:
> >> >> "Dr. David Alan Gilbert" <dgilb...@redhat.com> writes:
> >> >>
> >> >> > * Markus Armbruster (arm...@redhat.com) wrote:
> >> >> >> "Dr. David Alan Gilbert (git)" <dgilb...@redhat.com> writes:
> >> >> >>
> >> >> >> > From: "Dr. David Alan Gilbert" <dgilb...@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 <afaer...@suse.de>
> >> >> >> >
> >> >> >> > Updated for changed interface:
> >> >> >> > Signed-off-by: Dr. David Alan Gilbert <dgilb...@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 / dgilb...@redhat.com / Manchester, UK
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
