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

[Qemu-devel] [PATCH for-2.9 v2] 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>
---
v2: - input visitor to reject non-empty strings

This is needed for David's patch to work:

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

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

Since this v2 changes the behavior to reject non-empty null properties,
it is up to getset_compat_deprecated() to ignore the error. The following
folded into patch [RFCv2 10/12] does the trick:

--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -8446,7 +8446,7 @@ static void getset_compat_deprecated(Object *obj, Visitor
     if (!qtest_enabled()) {
         error_report("CPU 'compat' property is deprecated and has no effect; us
     }
-    visit_type_null(v, name, errp);
+    visit_type_null(v, name, NULL);
 }

 static PropertyInfo ppc_compat_deprecated_propinfo = {
---
 qapi/string-input-visitor.c  |   11 +++++++++++
 qapi/string-output-visitor.c |   14 ++++++++++++++
 2 files changed, 25 insertions(+)

diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
index 8dfa5612522b..9e9d2a1d2865 100644
--- a/qapi/string-input-visitor.c
+++ b/qapi/string-input-visitor.c
@@ -314,6 +314,16 @@ 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)
+{
+    StringInputVisitor *siv = to_siv(v);
+
+    if (siv->string && siv->string[0]) {
+        error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
+                   "null");
+    }
+}
+
 static void parse_optional(Visitor *v, const char *name, bool *present)
 {
     StringInputVisitor *siv = to_siv(v);
@@ -348,6 +358,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 for-2.9 v2] 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>
> ---
> v2: - input visitor to reject non-empty strings
>
> This is needed for David's patch to work:
>
> [RFCv2 10/12] pseries: Move CPU compatibility property to machine

Recommend to insert this patch less the fixup to David's patch before
David's patch, then squash in the fixup, if it's needed.

> Messag-Id: <1479248275-18889-11-git-send-email-david@gibson.dropbear.id.au>
>
> Since this v2 changes the behavior to reject non-empty null properties,
> it is up to getset_compat_deprecated() to ignore the error. The following
> folded into patch [RFCv2 10/12] does the trick:
>
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c
> @@ -8446,7 +8446,7 @@ static void getset_compat_deprecated(Object *obj, Visitor
>      if (!qtest_enabled()) {
>          error_report("CPU 'compat' property is deprecated and has no effect; us
>      }
> -    visit_type_null(v, name, errp);
> +    visit_type_null(v, name, NULL);

Are you sure we want to ignore errors here?

>  }
>
>  static PropertyInfo ppc_compat_deprecated_propinfo = {
> ---
>  qapi/string-input-visitor.c  |   11 +++++++++++
>  qapi/string-output-visitor.c |   14 ++++++++++++++
>  2 files changed, 25 insertions(+)
>
> diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
> index 8dfa5612522b..9e9d2a1d2865 100644
> --- a/qapi/string-input-visitor.c
> +++ b/qapi/string-input-visitor.c
> @@ -314,6 +314,16 @@ 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)
> +{
> +    StringInputVisitor *siv = to_siv(v);
> +
> +    if (siv->string && siv->string[0]) {
> +        error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
> +                   "null");
> +    }
> +}

When !siv->string, the other parse_type_FOO() generally do

    error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
               ... a description of the expected type...);

This one doesn't.  Why?

Should the conditional be !siv->string || siv->string[0]?

> +
>  static void parse_optional(Visitor *v, const char *name, bool *present)
>  {
>      StringInputVisitor *siv = to_siv(v);
> @@ -348,6 +358,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 for-2.9 v2] qapi: add explicit null to string input and output visitors

[David Gibson]

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

On Thu, Dec 15, 2016 at 10:44:02AM +0100, Markus Armbruster 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>
> > ---
> > v2: - input visitor to reject non-empty strings
> >
> > This is needed for David's patch to work:
> >
> > [RFCv2 10/12] pseries: Move CPU compatibility property to machine
> 
> Recommend to insert this patch less the fixup to David's patch before
> David's patch, then squash in the fixup, if it's needed.

That series of mine needs a bunch of rework for several things
including this.  It'll probably be a while before I can get back to
that, so don't hold this up on my account.

> 
> > Messag-Id: <1479248275-18889-11-git-send-email-david@gibson.dropbear.id.au>
> >
> > Since this v2 changes the behavior to reject non-empty null properties,
> > it is up to getset_compat_deprecated() to ignore the error. The following
> > folded into patch [RFCv2 10/12] does the trick:
> >
> > --- a/target-ppc/translate_init.c
> > +++ b/target-ppc/translate_init.c
> > @@ -8446,7 +8446,7 @@ static void getset_compat_deprecated(Object *obj, Visitor
> >      if (!qtest_enabled()) {
> >          error_report("CPU 'compat' property is deprecated and has no effect; us
> >      }
> > -    visit_type_null(v, name, errp);
> > +    visit_type_null(v, name, NULL);
> 
> Are you sure we want to ignore errors here?
> 
> >  }
> >
> >  static PropertyInfo ppc_compat_deprecated_propinfo = {
> > ---
> >  qapi/string-input-visitor.c  |   11 +++++++++++
> >  qapi/string-output-visitor.c |   14 ++++++++++++++
> >  2 files changed, 25 insertions(+)
> >
> > diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
> > index 8dfa5612522b..9e9d2a1d2865 100644
> > --- a/qapi/string-input-visitor.c
> > +++ b/qapi/string-input-visitor.c
> > @@ -314,6 +314,16 @@ 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)
> > +{
> > +    StringInputVisitor *siv = to_siv(v);
> > +
> > +    if (siv->string && siv->string[0]) {
> > +        error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
> > +                   "null");
> > +    }
> > +}
> 
> When !siv->string, the other parse_type_FOO() generally do
> 
>     error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
>                ... a description of the expected type...);
> 
> This one doesn't.  Why?
> 
> Should the conditional be !siv->string || siv->string[0]?
> 
> > +
> >  static void parse_optional(Visitor *v, const char *name, bool *present)
> >  {
> >      StringInputVisitor *siv = to_siv(v);
> > @@ -348,6 +358,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;
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Greg Kurz]

On Thu, 15 Dec 2016 10:44:02 +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>
> > ---
> > v2: - input visitor to reject non-empty strings
> >
> > This is needed for David's patch to work:
> >
> > [RFCv2 10/12] pseries: Move CPU compatibility property to machine  
> 
> Recommend to insert this patch less the fixup to David's patch before
> David's patch, then squash in the fixup, if it's needed.
> 

This is exactly the state of my test tree. :)

> > Messag-Id: <1479248275-18889-11-git-send-email-david@gibson.dropbear.id.au>
> >
> > Since this v2 changes the behavior to reject non-empty null properties,
> > it is up to getset_compat_deprecated() to ignore the error. The following
> > folded into patch [RFCv2 10/12] does the trick:
> >
> > --- a/target-ppc/translate_init.c
> > +++ b/target-ppc/translate_init.c
> > @@ -8446,7 +8446,7 @@ static void getset_compat_deprecated(Object *obj, Visitor
> >      if (!qtest_enabled()) {
> >          error_report("CPU 'compat' property is deprecated and has no effect; us
> >      }
> > -    visit_type_null(v, name, errp);
> > +    visit_type_null(v, name, NULL);  
> 
> Are you sure we want to ignore errors here?
> 

The following lines come from the changelog of David's patch [RFCv2 10/12]:
---
To reflect this, this removes the CPU 'compat' property and instead
creates a 'max-cpu-compat' property on the pseries machine.  Strictly
speaking this breaks compatibility, but AFAIK the 'compat' option was
never (directly) used with -device or device_add.

The option was used with -cpu.  So, to maintain compatibility, this patch
adds a hack to the cpu option parsing to strip out any compat options
supplied with -cpu and set them on the machine property instead of the new
removed cpu property.
---

i.e. we want to keep -cpu compat=blah because we turn it internally into
-machine max-cpu-compat=blah, and we want the 'compat' property to be
simply ignored by cpu objects.

visit_type_null() can only fail when setting a non-empty 'compat', which
is precisely the case we want to ignore.

> >  }
> >
> >  static PropertyInfo ppc_compat_deprecated_propinfo = {
> > ---
> >  qapi/string-input-visitor.c  |   11 +++++++++++
> >  qapi/string-output-visitor.c |   14 ++++++++++++++
> >  2 files changed, 25 insertions(+)
> >
> > diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
> > index 8dfa5612522b..9e9d2a1d2865 100644
> > --- a/qapi/string-input-visitor.c
> > +++ b/qapi/string-input-visitor.c
> > @@ -314,6 +314,16 @@ 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)
> > +{
> > +    StringInputVisitor *siv = to_siv(v);
> > +
> > +    if (siv->string && siv->string[0]) {
> > +        error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
> > +                   "null");
> > +    }
> > +}  
> 
> When !siv->string, the other parse_type_FOO() generally do
> 
>     error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
>                ... a description of the expected type...);
> 
> This one doesn't.  Why?
> 
> Should the conditional be !siv->string || siv->string[0]?
> 

Yes, you're right.

> > +
> >  static void parse_optional(Visitor *v, const char *name, bool *present)
> >  {
> >      StringInputVisitor *siv = to_siv(v);
> > @@ -348,6 +358,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;