From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57495) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1blzS3-0008UX-Eh for qemu-devel@nongnu.org; Mon, 19 Sep 2016 10:21:44 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1blzRz-00089x-7k for qemu-devel@nongnu.org; Mon, 19 Sep 2016 10:21:42 -0400 Received: from mx1.redhat.com ([209.132.183.28]:42342) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1blzRy-00089h-UU for qemu-devel@nongnu.org; Mon, 19 Sep 2016 10:21:39 -0400 Date: Mon, 19 Sep 2016 15:21:34 +0100 From: "Dr. David Alan Gilbert" Message-ID: <20160919142133.GD2041@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> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <871t0gdlj0.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 (git)" writes: > >>=20 > >> > From: "Dr. David Alan Gilbert" > >> > > >> > 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=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-output-vis= itor.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, con= st 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 implemen= t. > >> See the comment in string-output-visitor.h. > >>=20 > >> String input visitor and options visitor have similar holes; see the > >> comments in string-input-visitor.h and opts-visitor.h. > >>=20 > >> Should we change all of them together to report errors instead of cr= ash? > >> With common "error out because this isn't implemented" methods? > > > > In that case wouldn't it be best to change visit_start_struct/visit_e= nd_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 implem= entation, now you have to go around and fix all the visitor implementations to init= ialise 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