Message ID | 1472117833-10236-2-git-send-email-dgilbert@redhat.com |
---|---|
State | New |
Headers | show |
"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
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) +{ +} + 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; }