[Qemu-devel] [PATCH] qapi: add explicit null to string input and output visitors

[Qemu-devel] [PATCH] qapi: add explicit null to string input and output visitors

[Greg Kurz]

This may be used for deprecated object properties that are kept for
backwards compatibility.

Signed-off-by: Greg Kurz <groug@kaod.org>
---

This is enough to fix David's patch:

[RFCv2 10/12] pseries: Move CPU compatibility property to machine

Messag-Id: <1479248275-18889-11-git-send-email-david@gibson.dropbear.id.au>

 qapi/string-input-visitor.c  |    5 +++++
 qapi/string-output-visitor.c |   14 ++++++++++++++
 2 files changed, 19 insertions(+)

diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
index 8dfa5612522b..4a378d7b67e6 100644
--- a/qapi/string-input-visitor.c
+++ b/qapi/string-input-visitor.c
@@ -314,6 +314,10 @@ static void parse_type_number(Visitor *v, const char *name, double *obj,
     *obj = val;
 }
 
+static void parse_type_null(Visitor *v, const char *name, Error **errp)
+{
+}
+
 static void parse_optional(Visitor *v, const char *name, bool *present)
 {
     StringInputVisitor *siv = to_siv(v);
@@ -348,6 +352,7 @@ Visitor *string_input_visitor_new(const char *str)
     v->visitor.type_bool = parse_type_bool;
     v->visitor.type_str = parse_type_str;
     v->visitor.type_number = parse_type_number;
+    v->visitor.type_null = parse_type_null;
     v->visitor.start_list = start_list;
     v->visitor.next_list = next_list;
     v->visitor.end_list = end_list;
diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c
index 94ac8211d144..5ec5352ca87c 100644
--- a/qapi/string-output-visitor.c
+++ b/qapi/string-output-visitor.c
@@ -266,6 +266,19 @@ static void print_type_number(Visitor *v, const char *name, double *obj,
     string_output_set(sov, g_strdup_printf("%f", *obj));
 }
 
+static void print_type_null(Visitor *v, const char *name, Error **errp)
+{
+    StringOutputVisitor *sov = to_sov(v);
+    char *out;
+
+    if (sov->human) {
+        out = g_strdup("<null>");
+    } else {
+        out = g_strdup("");
+    }
+    string_output_set(sov, out);
+}
+
 static void
 start_list(Visitor *v, const char *name, GenericList **list, size_t size,
            Error **errp)
@@ -351,6 +364,7 @@ Visitor *string_output_visitor_new(bool human, char **result)
     v->visitor.type_bool = print_type_bool;
     v->visitor.type_str = print_type_str;
     v->visitor.type_number = print_type_number;
+    v->visitor.type_null = print_type_null;
     v->visitor.start_list = start_list;
     v->visitor.next_list = next_list;
     v->visitor.end_list = end_list;

Re: [Qemu-devel] [PATCH] qapi: add explicit null to string input and output visitors

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 3330 bytes --]

On 11/19/2016 02:28 AM, Greg Kurz wrote:
> This may be used for deprecated object properties that are kept for
> backwards compatibility.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
> 
> This is enough to fix David's patch:
> 
> [RFCv2 10/12] pseries: Move CPU compatibility property to machine

Do we need both input and output visitors, or is the problem only caused
on an output visit?

> 
> Messag-Id: <1479248275-18889-11-git-send-email-david@gibson.dropbear.id.au>
> 
>  qapi/string-input-visitor.c  |    5 +++++
>  qapi/string-output-visitor.c |   14 ++++++++++++++
>  2 files changed, 19 insertions(+)
> 
> diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
> index 8dfa5612522b..4a378d7b67e6 100644
> --- a/qapi/string-input-visitor.c
> +++ b/qapi/string-input-visitor.c
> @@ -314,6 +314,10 @@ static void parse_type_number(Visitor *v, const char *name, double *obj,
>      *obj = val;
>  }
>  
> +static void parse_type_null(Visitor *v, const char *name, Error **errp)
> +{
> +}

In particular, this always succeeds, even though we don't have a
symmetric way to guarantee that the user can specifically request NULL
input (the qobject visitor checks for the JSON keyword NULL, but that's
different from string parsing), so I'm a bit worried that it is not
doing what we want. If you omit this hunk, does the problem with David's
patch still go away with just the output visitor change?

> +
>  static void parse_optional(Visitor *v, const char *name, bool *present)
>  {
>      StringInputVisitor *siv = to_siv(v);
> @@ -348,6 +352,7 @@ Visitor *string_input_visitor_new(const char *str)
>      v->visitor.type_bool = parse_type_bool;
>      v->visitor.type_str = parse_type_str;
>      v->visitor.type_number = parse_type_number;
> +    v->visitor.type_null = parse_type_null;
>      v->visitor.start_list = start_list;
>      v->visitor.next_list = next_list;
>      v->visitor.end_list = end_list;
> diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c
> index 94ac8211d144..5ec5352ca87c 100644
> --- a/qapi/string-output-visitor.c
> +++ b/qapi/string-output-visitor.c
> @@ -266,6 +266,19 @@ static void print_type_number(Visitor *v, const char *name, double *obj,
>      string_output_set(sov, g_strdup_printf("%f", *obj));
>  }
>  
> +static void print_type_null(Visitor *v, const char *name, Error **errp)
> +{
> +    StringOutputVisitor *sov = to_sov(v);
> +    char *out;
> +
> +    if (sov->human) {
> +        out = g_strdup("<null>");
> +    } else {
> +        out = g_strdup("");
> +    }
> +    string_output_set(sov, out);
> +}
> +
>  static void
>  start_list(Visitor *v, const char *name, GenericList **list, size_t size,
>             Error **errp)
> @@ -351,6 +364,7 @@ Visitor *string_output_visitor_new(bool human, char **result)
>      v->visitor.type_bool = print_type_bool;
>      v->visitor.type_str = print_type_str;
>      v->visitor.type_number = print_type_number;
> +    v->visitor.type_null = print_type_null;
>      v->visitor.start_list = start_list;
>      v->visitor.next_list = next_list;
>      v->visitor.end_list = end_list;
> 
> 
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH] qapi: add explicit null to string input and output visitors

[Greg Kurz]

[-- Attachment #1: Type: text/plain, Size: 4105 bytes --]

On Mon, 21 Nov 2016 10:32:50 -0600
Eric Blake <eblake@redhat.com> wrote:

> On 11/19/2016 02:28 AM, Greg Kurz wrote:
> > This may be used for deprecated object properties that are kept for
> > backwards compatibility.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> > 
> > This is enough to fix David's patch:
> > 
> > [RFCv2 10/12] pseries: Move CPU compatibility property to machine  
> 
> Do we need both input and output visitors, or is the problem only caused
> on an output visit?
> 

David's patch unifies the get/set property info callbacks into a single
getset_compat_deprecated() function which calls visit_type_null(). So,
even if the operation is a nop for input visitors, we need it anyway,
otherwise visit_type_null() dereferences a NULL pointer.

> > 
> > Messag-Id: <1479248275-18889-11-git-send-email-david@gibson.dropbear.id.au>
> > 
> >  qapi/string-input-visitor.c  |    5 +++++
> >  qapi/string-output-visitor.c |   14 ++++++++++++++
> >  2 files changed, 19 insertions(+)
> > 
> > diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
> > index 8dfa5612522b..4a378d7b67e6 100644
> > --- a/qapi/string-input-visitor.c
> > +++ b/qapi/string-input-visitor.c
> > @@ -314,6 +314,10 @@ static void parse_type_number(Visitor *v, const char *name, double *obj,
> >      *obj = val;
> >  }
> >  
> > +static void parse_type_null(Visitor *v, const char *name, Error **errp)
> > +{
> > +}  
> 
> In particular, this always succeeds, even though we don't have a
> symmetric way to guarantee that the user can specifically request NULL
> input (the qobject visitor checks for the JSON keyword NULL, but that's
> different from string parsing), so I'm a bit worried that it is not
> doing what we want. If you omit this hunk, does the problem with David's
> patch still go away with just the output visitor change?
> 

What we want here is to simply ignore any string the user would pass.

As explained above, this hunk is needed as long as David's patch calls
visit_type_null() for both input and output. Otherwise, it should have
separate functions: get_compat_deprecated() which calls visit_type_null()
and set_compat_deprecated() which does nothing.

> > +
> >  static void parse_optional(Visitor *v, const char *name, bool *present)
> >  {
> >      StringInputVisitor *siv = to_siv(v);
> > @@ -348,6 +352,7 @@ Visitor *string_input_visitor_new(const char *str)
> >      v->visitor.type_bool = parse_type_bool;
> >      v->visitor.type_str = parse_type_str;
> >      v->visitor.type_number = parse_type_number;
> > +    v->visitor.type_null = parse_type_null;
> >      v->visitor.start_list = start_list;
> >      v->visitor.next_list = next_list;
> >      v->visitor.end_list = end_list;
> > diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c
> > index 94ac8211d144..5ec5352ca87c 100644
> > --- a/qapi/string-output-visitor.c
> > +++ b/qapi/string-output-visitor.c
> > @@ -266,6 +266,19 @@ static void print_type_number(Visitor *v, const char *name, double *obj,
> >      string_output_set(sov, g_strdup_printf("%f", *obj));
> >  }
> >  
> > +static void print_type_null(Visitor *v, const char *name, Error **errp)
> > +{
> > +    StringOutputVisitor *sov = to_sov(v);
> > +    char *out;
> > +
> > +    if (sov->human) {
> > +        out = g_strdup("<null>");
> > +    } else {
> > +        out = g_strdup("");
> > +    }
> > +    string_output_set(sov, out);
> > +}
> > +
> >  static void
> >  start_list(Visitor *v, const char *name, GenericList **list, size_t size,
> >             Error **errp)
> > @@ -351,6 +364,7 @@ Visitor *string_output_visitor_new(bool human, char **result)
> >      v->visitor.type_bool = print_type_bool;
> >      v->visitor.type_str = print_type_str;
> >      v->visitor.type_number = print_type_number;
> > +    v->visitor.type_null = print_type_null;
> >      v->visitor.start_list = start_list;
> >      v->visitor.next_list = next_list;
> >      v->visitor.end_list = end_list;
> > 
> > 
> >   
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [Qemu-devel] [PATCH] qapi: add explicit null to string input and output visitors

[Markus Armbruster]

Greg Kurz <groug@kaod.org> writes:

> This may be used for deprecated object properties that are kept for
> backwards compatibility.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>
> This is enough to fix David's patch:
>
> [RFCv2 10/12] pseries: Move CPU compatibility property to machine
>
> Messag-Id: <1479248275-18889-11-git-send-email-david@gibson.dropbear.id.au>
>
>  qapi/string-input-visitor.c  |    5 +++++
>  qapi/string-output-visitor.c |   14 ++++++++++++++
>  2 files changed, 19 insertions(+)
>
> diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
> index 8dfa5612522b..4a378d7b67e6 100644
> --- a/qapi/string-input-visitor.c
> +++ b/qapi/string-input-visitor.c
> @@ -314,6 +314,10 @@ static void parse_type_number(Visitor *v, const char *name, double *obj,
>      *obj = val;
>  }
>  
> +static void parse_type_null(Visitor *v, const char *name, Error **errp)
> +{
> +}
> +

I figure this accepts an arbitrary string.  The output visitor ...


>  static void parse_optional(Visitor *v, const char *name, bool *present)
>  {
>      StringInputVisitor *siv = to_siv(v);
> @@ -348,6 +352,7 @@ Visitor *string_input_visitor_new(const char *str)
>      v->visitor.type_bool = parse_type_bool;
>      v->visitor.type_str = parse_type_str;
>      v->visitor.type_number = parse_type_number;
> +    v->visitor.type_null = parse_type_null;
>      v->visitor.start_list = start_list;
>      v->visitor.next_list = next_list;
>      v->visitor.end_list = end_list;
> diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c
> index 94ac8211d144..5ec5352ca87c 100644
> --- a/qapi/string-output-visitor.c
> +++ b/qapi/string-output-visitor.c
> @@ -266,6 +266,19 @@ static void print_type_number(Visitor *v, const char *name, double *obj,
>      string_output_set(sov, g_strdup_printf("%f", *obj));
>  }
>  
> +static void print_type_null(Visitor *v, const char *name, Error **errp)
> +{
> +    StringOutputVisitor *sov = to_sov(v);
> +    char *out;
> +
> +    if (sov->human) {
> +        out = g_strdup("<null>");
> +    } else {
> +        out = g_strdup("");

... produces an empty string.  Sure this makes sense?

Shouldn't the input visitor reject anything but the empty string?

> +    }
> +    string_output_set(sov, out);
> +}
> +
>  static void
>  start_list(Visitor *v, const char *name, GenericList **list, size_t size,
>             Error **errp)
> @@ -351,6 +364,7 @@ Visitor *string_output_visitor_new(bool human, char **result)
>      v->visitor.type_bool = print_type_bool;
>      v->visitor.type_str = print_type_str;
>      v->visitor.type_number = print_type_number;
> +    v->visitor.type_null = print_type_null;
>      v->visitor.start_list = start_list;
>      v->visitor.next_list = next_list;
>      v->visitor.end_list = end_list;

Re: [Qemu-devel] [PATCH] qapi: add explicit null to string input and output visitors

[Greg Kurz]

On Tue, 13 Dec 2016 09:43:41 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Greg Kurz <groug@kaod.org> writes:
> 
> > This may be used for deprecated object properties that are kept for
> > backwards compatibility.
> >
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >
> > This is enough to fix David's patch:
> >
> > [RFCv2 10/12] pseries: Move CPU compatibility property to machine
> >
> > Messag-Id: <1479248275-18889-11-git-send-email-david@gibson.dropbear.id.au>
> >
> >  qapi/string-input-visitor.c  |    5 +++++
> >  qapi/string-output-visitor.c |   14 ++++++++++++++
> >  2 files changed, 19 insertions(+)
> >
> > diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
> > index 8dfa5612522b..4a378d7b67e6 100644
> > --- a/qapi/string-input-visitor.c
> > +++ b/qapi/string-input-visitor.c
> > @@ -314,6 +314,10 @@ static void parse_type_number(Visitor *v, const char *name, double *obj,
> >      *obj = val;
> >  }
> >  
> > +static void parse_type_null(Visitor *v, const char *name, Error **errp)
> > +{
> > +}
> > +  
> 
> I figure this accepts an arbitrary string.  The output visitor ...
> 

Yes, I assumed that null means "we don't care"... but maybe I'm wrong. :)

> 
> >  static void parse_optional(Visitor *v, const char *name, bool *present)
> >  {
> >      StringInputVisitor *siv = to_siv(v);
> > @@ -348,6 +352,7 @@ Visitor *string_input_visitor_new(const char *str)
> >      v->visitor.type_bool = parse_type_bool;
> >      v->visitor.type_str = parse_type_str;
> >      v->visitor.type_number = parse_type_number;
> > +    v->visitor.type_null = parse_type_null;
> >      v->visitor.start_list = start_list;
> >      v->visitor.next_list = next_list;
> >      v->visitor.end_list = end_list;
> > diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c
> > index 94ac8211d144..5ec5352ca87c 100644
> > --- a/qapi/string-output-visitor.c
> > +++ b/qapi/string-output-visitor.c
> > @@ -266,6 +266,19 @@ static void print_type_number(Visitor *v, const char *name, double *obj,
> >      string_output_set(sov, g_strdup_printf("%f", *obj));
> >  }
> >  
> > +static void print_type_null(Visitor *v, const char *name, Error **errp)
> > +{
> > +    StringOutputVisitor *sov = to_sov(v);
> > +    char *out;
> > +
> > +    if (sov->human) {
> > +        out = g_strdup("<null>");
> > +    } else {
> > +        out = g_strdup("");  
> 
> ... produces an empty string.  Sure this makes sense?
> 
> Shouldn't the input visitor reject anything but the empty string?
> 

Looking at how null is handled for qobject, I suppose you're right. I'll
send a v2.

> > +    }
> > +    string_output_set(sov, out);
> > +}
> > +
> >  static void
> >  start_list(Visitor *v, const char *name, GenericList **list, size_t size,
> >             Error **errp)
> > @@ -351,6 +364,7 @@ Visitor *string_output_visitor_new(bool human, char **result)
> >      v->visitor.type_bool = print_type_bool;
> >      v->visitor.type_str = print_type_str;
> >      v->visitor.type_number = print_type_number;
> > +    v->visitor.type_null = print_type_null;
> >      v->visitor.start_list = start_list;
> >      v->visitor.next_list = next_list;
> >      v->visitor.end_list = end_list;