| Message ID | 20160921103629.6410-3-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed 21 Sep 2016 12:36:28 PM CEST, Marc-André Lureau wrote: > Some getters already set *obj argument to NULL early, let's do this for > all for consistent behaviour in case of errors. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> Reviewed-by: Alberto Garcia <berto@igalia.com> Berto
On Wed, Sep 21, 2016 at 02:36:28PM +0400, Marc-André Lureau wrote: > Some getters already set *obj argument to NULL early, let's do this for > all for consistent behaviour in case of errors. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> If we want consistent behaviour, there's plenty more visit methods that need updating beyond these two. eg input_type_int64 will leave '*obj' untouched on error. In fact if we want to have '*obj' given a NULL value on error, then it seems we should instead add code to 'qapi-visit-core.c' to always initialize '*obj' to NULL, instead of doing it in qmp-input-visitor.c That way all visitor implementations get the same behaviour Alternatively, we could say that '*obj' should never be touched on error paths, in which case we've a bunchof cleanup todo in qmp_input_visitor to avoid splattering *obj. Regards, Daniel
Hi ----- Original Message ----- > On Wed, Sep 21, 2016 at 02:36:28PM +0400, Marc-André Lureau wrote: > > Some getters already set *obj argument to NULL early, let's do this for > > all for consistent behaviour in case of errors. > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > If we want consistent behaviour, there's plenty more visit methods > that need updating beyond these two. eg input_type_int64 will > leave '*obj' untouched on error. > > In fact if we want to have '*obj' given a NULL value on error, > then it seems we should instead add code to 'qapi-visit-core.c' > to always initialize '*obj' to NULL, instead of doing it in > qmp-input-visitor.c That way all visitor implementations get > the same behaviour I think that's not easily doable, as an input visitor will want to set *obj (to NULL or something), but the output visitor may need *obj != NULL as an input. It'snot really elegant that there is visitor-input/output specific code already in the visit-core, I would rather have that code in the respective visitors. > > Alternatively, we could say that '*obj' should never be touched > on error paths, in which case we've a bunchof cleanup todo > in qmp_input_visitor to avoid splattering *obj. > > Regards, > Daniel > -- > |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| > |: http://libvirt.org -o- http://virt-manager.org :| > |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| > |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| >
Marc-André Lureau <marcandre.lureau@redhat.com> writes: > Some getters already set *obj argument to NULL early, let's do this for > all for consistent behaviour in case of errors. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > qapi/qmp-input-visitor.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c > index ea9972d..cb9d196 100644 > --- a/qapi/qmp-input-visitor.c > +++ b/qapi/qmp-input-visitor.c > @@ -322,11 +322,13 @@ static void qmp_input_type_str(Visitor *v, const char *name, char **obj, > QObject *qobj = qmp_input_get_object(qiv, name, true, errp); > QString *qstr = qobject_to_qstring(qobj); > > + if (obj) { > + *obj = NULL; > + } > if (!qobj) { > return; > } > if (!qstr) { > - *obj = NULL; > error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", > "string"); > return; This undoes damage done in PATCH 1. > @@ -368,6 +370,9 @@ static void qmp_input_type_any(Visitor *v, const char *name, QObject **obj, > QmpInputVisitor *qiv = to_qiv(v); > QObject *qobj = qmp_input_get_object(qiv, name, true, errp); > > + if (obj) { > + *obj = NULL; > + } > if (!qobj) { > return; > } Likewise. Similar damage done to qmp_input_start_list() and possibly others. Please squash into PATCH 1 and double-check your new error returns affect *obj like the existing ones.
On Wed, Sep 21, 2016 at 11:17:45AM -0400, Marc-André Lureau wrote: > Hi > > ----- Original Message ----- > > On Wed, Sep 21, 2016 at 02:36:28PM +0400, Marc-André Lureau wrote: > > > Some getters already set *obj argument to NULL early, let's do this for > > > all for consistent behaviour in case of errors. > > > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > > > If we want consistent behaviour, there's plenty more visit methods > > that need updating beyond these two. eg input_type_int64 will > > leave '*obj' untouched on error. > > > > In fact if we want to have '*obj' given a NULL value on error, > > then it seems we should instead add code to 'qapi-visit-core.c' > > to always initialize '*obj' to NULL, instead of doing it in > > qmp-input-visitor.c That way all visitor implementations get > > the same behaviour > > I think that's not easily doable, as an input visitor will want to set *obj (to NULL or something), but the output visitor may need *obj != NULL as an input. Oh good point. > It'snot really elegant that there is visitor-input/output specific code already in the visit-core, I would rather have that code in the respective visitors. Also, my series of visitor patches will delete opts-visitor and string-input-visitor, so ultimately qmp-input-visitor will be the only one left doing input work. Regards, Daniel
diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c index ea9972d..cb9d196 100644 --- a/qapi/qmp-input-visitor.c +++ b/qapi/qmp-input-visitor.c @@ -322,11 +322,13 @@ static void qmp_input_type_str(Visitor *v, const char *name, char **obj, QObject *qobj = qmp_input_get_object(qiv, name, true, errp); QString *qstr = qobject_to_qstring(qobj); + if (obj) { + *obj = NULL; + } if (!qobj) { return; } if (!qstr) { - *obj = NULL; error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", "string"); return; @@ -368,6 +370,9 @@ static void qmp_input_type_any(Visitor *v, const char *name, QObject **obj, QmpInputVisitor *qiv = to_qiv(v); QObject *qobj = qmp_input_get_object(qiv, name, true, errp); + if (obj) { + *obj = NULL; + } if (!qobj) { return; }
Some getters already set *obj argument to NULL early, let's do this for all for consistent behaviour in case of errors. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- qapi/qmp-input-visitor.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)