From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58168) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bm2Xj-0003WV-GD for qemu-devel@nongnu.org; Mon, 19 Sep 2016 13:39:51 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bm2Xd-00042K-1v for qemu-devel@nongnu.org; Mon, 19 Sep 2016 13:39:46 -0400 Received: from mx1.redhat.com ([209.132.183.28]:42056) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bm2Xc-000427-PH for qemu-devel@nongnu.org; Mon, 19 Sep 2016 13:39:41 -0400 Date: Mon, 19 Sep 2016 18:39:36 +0100 From: "Dr. David Alan Gilbert" Message-ID: <20160919173935.GF2041@work-vm> References: <1472117833-10236-1-git-send-email-dgilbert@redhat.com> <1472117833-10236-2-git-send-email-dgilbert@redhat.com> <87wpi8f310.fsf@dusky.pond.sub.org> <20160919134736.GC2041@work-vm> <871t0gdlj0.fsf@dusky.pond.sub.org> <20160919142133.GD2041@work-vm> <8737kvc3lk.fsf@dusky.pond.sub.org> <20160919153831.GE2041@work-vm> <87vaxr7ry6.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <87vaxr7ry6.fsf@dusky.pond.sub.org> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 1/2] qapi: Stub out StringOutputVisitor struct support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: kwolf@redhat.com, qemu-devel@nongnu.org, lcapitulino@redhat.com, arei.gonglei@huawei.com, pbonzini@redhat.com, afaerber@suse.de * Markus Armbruster (armbru@redhat.com) wrote: > "Dr. David Alan Gilbert" writes: >=20 > > * Markus Armbruster (armbru@redhat.com) wrote: > >> "Dr. David Alan Gilbert" writes: > >>=20 > >> > * Markus Armbruster (armbru@redhat.com) wrote: > >> >> "Dr. David Alan Gilbert" writes: > >> >>=20 > >> >> > * Markus Armbruster (armbru@redhat.com) wrote: > >> >> >> "Dr. David Alan Gilbert (git)" writes: > >> >> >>=20 > >> >> >> > From: "Dr. David Alan Gilbert" > >> >> >> > > >> >> >> > Avoid a segfault when visiting, e.g., the QOM rtc-time prope= rty, > >> >> >> > by implementing the struct callbacks and raising an Error. > >> >> >> > > >> >> >> > Signed-off-by: Andreas F=E4rber > >> >> >> > > >> >> >> > Updated for changed interface: > >> >> >> > Signed-off-by: Dr. David Alan Gilbert > >> >> >> > --- > >> >> >> > qapi/string-output-visitor.c | 13 +++++++++++++ > >> >> >> > 1 file changed, 13 insertions(+) > >> >> >> > > >> >> >> > diff --git a/qapi/string-output-visitor.c b/qapi/string-outp= ut-visitor.c > >> >> >> > index 94ac821..4e7e97f 100644 > >> >> >> > --- a/qapi/string-output-visitor.c > >> >> >> > +++ b/qapi/string-output-visitor.c > >> >> >> > @@ -12,6 +12,7 @@ > >> >> >> > =20 > >> >> >> > #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)); > >> >> >> > } > >> >> >> > =20 > >> >> >> > +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) > >> >> >> > +{ > >> >> >> > +} > >> >> >> > + > >> >> >>=20 > >> >> >> This is just one of the several things this visitor doesn't im= plement. > >> >> >> See the comment in string-output-visitor.h. > >> >> >>=20 > >> >> >> String input visitor and options visitor have similar holes; s= ee the > >> >> >> comments in string-input-visitor.h and opts-visitor.h. > >> >> >>=20 > >> >> >> 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/v= isit_end_struct > >> >> > to do the check (Like visit_check_struct does). > >> >>=20 > >> >> In my opinion. > >> >>=20 > >> >> if (v->foo) { > >> >> v->foo(...); > >> >> } else { > >> >> ... default action ... > >> >> } > >> >>=20 > >> >> 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 wo= uldn't > >> > be a problem, and you wouldn't need most of the visit_ wrapper fun= ctions; > >> > but they don't, so the if (v->foo) { ... } else { error; } is th= e > >> > current cleanest we can do. > >>=20 > >> 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! >=20 > I know the feeling... >=20 > 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. >=20 > >> We currently have seven visitors. Every single one defines a > >> FOO_visitor_new() function that basically looks like this: > >>=20 > >> Visitor *FOO_visitor_new(... whatever ...) > >> { > >> FOOVisitor v =3D g_malloc0(sizeof(*v)); > >>=20 > >> v->visitor.type =3D ... > >> ... initialize more of v->visitor ... > >> ... initialize other members of *v, if any ... > >>=20 > >> return &v->visitor; > >> } > >>=20 > >> I grant you that putting sensible defaults into v->visitor by > >> initializing them correctly in all the FOO_visitor_new() functions i= s a > >> bit of pain. Not much pain; there are only seven. Anyway, there ar= e > >> several obvious ways to do this without pain: > >>=20 > >> (1) Have a visitor core function to set the defaults, call it first. > >>=20 > >> (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. >=20 > It's a fine way of doing it when you only ever create the things in one > way. Here, with g_malloc0(). >=20 > 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 defau= lts, > >> and call it last. This function can also check you filled out i= n > >> 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