Re: [Qemu-devel] [PATCH 2/3] qapi: clear given pointer

[Daniel P. Berrange]

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 
> > 
> > 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
-- 
|: 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 :|

Re: [Qemu-devel] [PATCH 2/3] qapi: clear given pointer

[Markus Armbruster]

Marc-André Lureau  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 
> ---
>  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.

Re: [Qemu-devel] [PATCH 2/3] qapi: clear given pointer

[Marc-André Lureau]

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 
> 
> 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 :|
> 

Re: [Qemu-devel] [PATCH 2/3] qapi: clear given pointer

[Daniel P. Berrange]

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 

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
-- 
|: 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 :|

Re: [Qemu-devel] [PATCH 2/3] qapi: clear given pointer

[Alberto Garcia]

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 

Reviewed-by: Alberto Garcia 

Berto

[Qemu-devel] [PATCH 2/3] qapi: clear given pointer

[Marc-André Lureau]

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 
---
 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;
@@ -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;
 }
-- 
2.10.0