[Qemu-devel] [PATCH 0/5] random qapi cleanups

[Qemu-devel] [PATCH 0/5] random qapi cleanups

[Eric Blake]

Trying to flush some of the random patches I have lying around
locally.  Some of these may have been posted to the list before,
perhaps as part of larger series, but I didn't bother to research
which parts came from where.

I'm not sure if this is in time for softfreeze; and if not, I'm
not sure how much is appropriate during freeze vs. being deferred
to 2.11.

Eric Blake (5):
  qapi: Further enhance visitor virtual walk doc example
  tests: Enhance qobject output to cover partial visit
  qapi: Visitor documentation tweak
  qtest: Avoid passing raw strings through hmp()
  qtest: Document calling conventions

 include/qapi/visitor.h              | 55 +++++++++++++++++++++++--------------
 tests/libqtest.h                    | 23 ++++++++++------
 tests/test-hmp.c                    |  4 +--
 tests/test-qobject-output-visitor.c | 46 ++++++++++++++++++++++++++++++-
 4 files changed, 96 insertions(+), 32 deletions(-)

-- 
2.9.4

[Qemu-devel] [PATCH 1/5] qapi: Further enhance visitor virtual walk doc example

[Eric Blake]

Markus pointed out that the example given for virtual walks did
not discuss how to do a virtual walk of an alternate type.  It
turns out that for output, we don't need to visit an alternate
(just directly visit the type that we want); and for input,
visit_start_alternate() is not currently wired up for alternates
(it requires a QAPI type, rather than permitting NULL for a
virtual walk).  Also, the example was never updated in commit
3b098d5 about where visit_complete() would fit in.  Improve the
description and example to give more details along these lines.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 include/qapi/visitor.h | 41 ++++++++++++++++++++++++++++-------------
 1 file changed, 28 insertions(+), 13 deletions(-)

diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index 74768aa..b0a048f 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -183,19 +183,25 @@
  *
  * It is also possible to use the visitors to do a virtual walk, where
  * no actual QAPI struct is present.  In this situation, decisions
- * about what needs to be walked are made by the calling code, and
- * structured visits are split between pairs of start and end methods
- * (where the end method must be called if the start function
- * succeeded, even if an intermediate visit encounters an error).
- * Thus, a virtual walk corresponding to '{ "list": [1, 2] }' looks
- * like:
+ * about what needs to be walked are made by the calling code (that
+ * is, there is no use for QAPI alternate types in a virtual walk,
+ * because the code can just directly visit the appropriate type
+ * within the alternate), and structured visits are split between
+ * pairs of start and end methods (where the end method must be called
+ * if the start function succeeded, even if an intermediate visit
+ * encounters an error).  Thus, a virtual output walk of an object
+ * containing a list of alternates between an integer or nested
+ * object, corresponding to '{ "list": [1, { "value": "2" } ] }',
+ * would look like:
  *
  * <example>
  *  Visitor *v;
  *  Error *err = NULL;
- *  int value;
+ *  int i = 1;
+ *  const char *s = "2";
+ *  FOO output;
  *
- *  v = FOO_visitor_new(...);
+ *  v = FOO_visitor_new(..., &output);
  *  visit_start_struct(v, NULL, NULL, 0, &err);
  *  if (err) {
  *      goto out;
@@ -204,16 +210,21 @@
  *  if (err) {
  *      goto outobj;
  *  }
- *  value = 1;
- *  visit_type_int(v, NULL, &value, &err);
+ *  visit_type_int(v, NULL, &i, &err);
  *  if (err) {
  *      goto outlist;
  *  }
- *  value = 2;
- *  visit_type_int(v, NULL, &value, &err);
+ *  visit_type_start(v, NULL, NULL, 0, &err);
  *  if (err) {
- *      goto outlist;
+ *      goto outnest;
+ *  }
+ *  visit_type_str(v, "value", (char **)&s, &err);
+ *  if (err) {
+ *      goto outnest;
  *  }
+ *  visit_check_struct(v, &err);
+ * outnest:
+ *  visit_end_struct(v, NULL);
  * outlist:
  *  visit_end_list(v, NULL);
  *  if (!err) {
@@ -221,6 +232,10 @@
  *  }
  * outobj:
  *  visit_end_struct(v, NULL);
+ *  if (!err) {
+ *      visit_complete(v, &output);
+ *      ...use output...
+ *  }
  * out:
  *  error_propagate(errp, err);
  *  visit_free(v);
-- 
2.9.4

Re: [Qemu-devel] [PATCH 1/5] qapi: Further enhance visitor virtual walk doc example

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> Markus pointed out that the example given for virtual walks did
> not discuss how to do a virtual walk of an alternate type.  It
> turns out that for output, we don't need to visit an alternate
> (just directly visit the type that we want); and for input,
> visit_start_alternate() is not currently wired up for alternates
> (it requires a QAPI type, rather than permitting NULL for a
> virtual walk).  Also, the example was never updated in commit
> 3b098d5 about where visit_complete() would fit in.  Improve the
> description and example to give more details along these lines.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>

This clashes with some unfinished work I have on alternates.  If I can
finish it quickly, we can compare and decide whether we still need this.

> ---
>  include/qapi/visitor.h | 41 ++++++++++++++++++++++++++++-------------
>  1 file changed, 28 insertions(+), 13 deletions(-)
>
> diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
> index 74768aa..b0a048f 100644
> --- a/include/qapi/visitor.h
> +++ b/include/qapi/visitor.h
> @@ -183,19 +183,25 @@
>   *
>   * It is also possible to use the visitors to do a virtual walk, where
>   * no actual QAPI struct is present.  In this situation, decisions
> - * about what needs to be walked are made by the calling code, and
> - * structured visits are split between pairs of start and end methods
> - * (where the end method must be called if the start function
> - * succeeded, even if an intermediate visit encounters an error).
> - * Thus, a virtual walk corresponding to '{ "list": [1, 2] }' looks
> - * like:
> + * about what needs to be walked are made by the calling code (that
> + * is, there is no use for QAPI alternate types in a virtual walk,
> + * because the code can just directly visit the appropriate type
> + * within the alternate), and structured visits are split between

The parenthesis makes a long sentence even longer.

> + * pairs of start and end methods (where the end method must be called
> + * if the start function succeeded, even if an intermediate visit
> + * encounters an error).  Thus, a virtual output walk of an object
> + * containing a list of alternates between an integer or nested
> + * object, corresponding to '{ "list": [1, { "value": "2" } ] }',
> + * would look like:

Covering alternates as well makes the example rather complicated.  In
the unfinished work I mentioned above, I explain them separately.

>   *
>   * <example>
>   *  Visitor *v;
>   *  Error *err = NULL;
> - *  int value;
> + *  int i = 1;
> + *  const char *s = "2";
> + *  FOO output;
>   *
> - *  v = FOO_visitor_new(...);
> + *  v = FOO_visitor_new(..., &output);
>   *  visit_start_struct(v, NULL, NULL, 0, &err);
>   *  if (err) {
>   *      goto out;
> @@ -204,16 +210,21 @@
>   *  if (err) {
>   *      goto outobj;
>   *  }
> - *  value = 1;
> - *  visit_type_int(v, NULL, &value, &err);
> + *  visit_type_int(v, NULL, &i, &err);
>   *  if (err) {
>   *      goto outlist;
>   *  }
> - *  value = 2;
> - *  visit_type_int(v, NULL, &value, &err);
> + *  visit_type_start(v, NULL, NULL, 0, &err);
>   *  if (err) {
> - *      goto outlist;
> + *      goto outnest;
> + *  }
> + *  visit_type_str(v, "value", (char **)&s, &err);
> + *  if (err) {
> + *      goto outnest;
>   *  }
> + *  visit_check_struct(v, &err);
> + * outnest:
> + *  visit_end_struct(v, NULL);
>   * outlist:
>   *  visit_end_list(v, NULL);
>   *  if (!err) {
> @@ -221,6 +232,10 @@
>   *  }
>   * outobj:
>   *  visit_end_struct(v, NULL);
> + *  if (!err) {
> + *      visit_complete(v, &output);
> + *      ...use output...
> + *  }
>   * out:
>   *  error_propagate(errp, err);
>   *  visit_free(v);

Re: [Qemu-devel] [PATCH 1/5] qapi: Further enhance visitor virtual walk doc example

[Eric Blake]

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

On 07/20/2017 04:05 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> Markus pointed out that the example given for virtual walks did
>> not discuss how to do a virtual walk of an alternate type.  It
>> turns out that for output, we don't need to visit an alternate
>> (just directly visit the type that we want); and for input,
>> visit_start_alternate() is not currently wired up for alternates
>> (it requires a QAPI type, rather than permitting NULL for a
>> virtual walk).  Also, the example was never updated in commit
>> 3b098d5 about where visit_complete() would fit in.  Improve the
>> description and example to give more details along these lines.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> This clashes with some unfinished work I have on alternates.  If I can
> finish it quickly, we can compare and decide whether we still need this.

It sounds like your stuff is 2.11 material; at this point, I'm trying to
decide which of my patches are still worth 2.10 softfreeze material.
While doc fixes are safe, I'm also fine stating that we don't want
churn, so I'll remove this part of my series from 2.10 consideration,
and like you say, we'll compare against your work in 2.11 later.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

[Qemu-devel] [PATCH 2/5] tests: Enhance qobject output to cover partial visit

[Eric Blake]

Add a test that proves (at least when run under valgrind) that
we are correctly handling allocated memory even when a visit
is aborted in the middle for whatever other reason.

See commit f24582d "qapi: fix double free in
qmp_output_visitor_cleanup()" for a fix that was lacking
testsuite exposure prior to this patch.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 tests/test-qobject-output-visitor.c | 46 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 45 insertions(+), 1 deletion(-)

diff --git a/tests/test-qobject-output-visitor.c b/tests/test-qobject-output-visitor.c
index 749c540..1e9a5d1 100644
--- a/tests/test-qobject-output-visitor.c
+++ b/tests/test-qobject-output-visitor.c
@@ -1,7 +1,7 @@
 /*
  * QObject Output Visitor unit-tests.
  *
- * Copyright (C) 2011-2016 Red Hat Inc.
+ * Copyright (C) 2011-2017 Red Hat Inc.
  *
  * Authors:
  *  Luiz Capitulino <lcapitulino@redhat.com>
@@ -251,6 +251,48 @@ static void test_visitor_out_struct_errors(TestOutputVisitorData *data,
 }


+static void test_visitor_out_partial_visit(TestOutputVisitorData *data,
+                                           const void *unused)
+{
+    /* Various checks that a mid-visit abort doesn't leak or double-free. */
+    const char *str = "hi";
+    Error *err = NULL;
+    UserDefAlternate uda = {
+        .type = QTYPE_QDICT,
+        .u.udfu = { .integer = 1,
+                    .string = (char *) "bye",
+                    .enum1 = -1 } /* intentionally bad */
+    };
+    UserDefAlternate *obj = &uda;
+
+    /* Abort within a nested object with no data members */
+    visit_start_struct(data->ov, NULL, NULL, 0, &error_abort);
+    visit_start_struct(data->ov, "nested", NULL, 0, &error_abort);
+    visitor_reset(data);
+
+    /* Abort in the middle of a list of strings */
+    visit_start_list(data->ov, "list", NULL, 0, &error_abort);
+    visit_type_str(data->ov, NULL, (char **)&str, &error_abort);
+    visit_type_str(data->ov, NULL, (char **)&str, &error_abort);
+    visitor_reset(data);
+
+    /*
+     * Abort in the middle of an alternate. Alternates can't be
+     * virtually visited, so we get to inline the first half of
+     * visit_type_UserDefAlternate().
+     */
+    visit_start_alternate(data->ov, NULL, (GenericAlternate **)&obj,
+                          sizeof(uda), &error_abort);
+    visit_start_struct(data->ov, NULL, NULL, 0, &error_abort);
+    visit_type_UserDefUnionBase_members(data->ov,
+                                        (UserDefUnionBase *)&uda.u.udfu,
+                                        &err);
+    /* error expected because of bad "enum1" discriminator value */
+    error_free_or_abort(&err);
+    visitor_reset(data);
+}
+
+
 static void test_visitor_out_list(TestOutputVisitorData *data,
                                   const void *unused)
 {
@@ -815,6 +857,8 @@ int main(int argc, char **argv)
                             &out_visitor_data, test_visitor_out_struct_nested);
     output_visitor_test_add("/visitor/output/struct-errors",
                             &out_visitor_data, test_visitor_out_struct_errors);
+    output_visitor_test_add("/visitor/output/partial-visit",
+                            &out_visitor_data, test_visitor_out_partial_visit);
     output_visitor_test_add("/visitor/output/list",
                             &out_visitor_data, test_visitor_out_list);
     output_visitor_test_add("/visitor/output/any",
-- 
2.9.4

Re: [Qemu-devel] [PATCH 2/5] tests: Enhance qobject output to cover partial visit

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> Add a test that proves (at least when run under valgrind) that
> we are correctly handling allocated memory even when a visit
> is aborted in the middle for whatever other reason.
>
> See commit f24582d "qapi: fix double free in
> qmp_output_visitor_cleanup()" for a fix that was lacking
> testsuite exposure prior to this patch.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  tests/test-qobject-output-visitor.c | 46 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 45 insertions(+), 1 deletion(-)
>
> diff --git a/tests/test-qobject-output-visitor.c b/tests/test-qobject-output-visitor.c
> index 749c540..1e9a5d1 100644
> --- a/tests/test-qobject-output-visitor.c
> +++ b/tests/test-qobject-output-visitor.c
> @@ -1,7 +1,7 @@
>  /*
>   * QObject Output Visitor unit-tests.
>   *
> - * Copyright (C) 2011-2016 Red Hat Inc.
> + * Copyright (C) 2011-2017 Red Hat Inc.
>   *
>   * Authors:
>   *  Luiz Capitulino <lcapitulino@redhat.com>
> @@ -251,6 +251,48 @@ static void test_visitor_out_struct_errors(TestOutputVisitorData *data,
>  }
>
>
> +static void test_visitor_out_partial_visit(TestOutputVisitorData *data,
> +                                           const void *unused)
> +{
> +    /* Various checks that a mid-visit abort doesn't leak or double-free. */
> +    const char *str = "hi";
> +    Error *err = NULL;
> +    UserDefAlternate uda = {
> +        .type = QTYPE_QDICT,
> +        .u.udfu = { .integer = 1,
> +                    .string = (char *) "bye",
> +                    .enum1 = -1 } /* intentionally bad */
> +    };
> +    UserDefAlternate *obj = &uda;
> +
> +    /* Abort within a nested object with no data members */
> +    visit_start_struct(data->ov, NULL, NULL, 0, &error_abort);
> +    visit_start_struct(data->ov, "nested", NULL, 0, &error_abort);
> +    visitor_reset(data);
> +
> +    /* Abort in the middle of a list of strings */
> +    visit_start_list(data->ov, "list", NULL, 0, &error_abort);
> +    visit_type_str(data->ov, NULL, (char **)&str, &error_abort);
> +    visit_type_str(data->ov, NULL, (char **)&str, &error_abort);
> +    visitor_reset(data);
> +
> +    /*
> +     * Abort in the middle of an alternate. Alternates can't be
> +     * virtually visited, so we get to inline the first half of
> +     * visit_type_UserDefAlternate().
> +     */

Not exactly inline.  Perhaps:

       /*
        * Abort in the middle of an alternate.  Since alternates don't
        * support virtual visits, we perform a real one, similar to what
        * visit_type_UserDefAlternate() would do.
        */

Hmm, what would visit_type_UserDefAlternate() do for @uda?  Could we
simply call it here and be done?

I've explored supporting virtual alternate visits, but my solution isn't
quite ready, yet.

> +    visit_start_alternate(data->ov, NULL, (GenericAlternate **)&obj,
> +                          sizeof(uda), &error_abort);
> +    visit_start_struct(data->ov, NULL, NULL, 0, &error_abort);
> +    visit_type_UserDefUnionBase_members(data->ov,
> +                                        (UserDefUnionBase *)&uda.u.udfu,
> +                                        &err);
> +    /* error expected because of bad "enum1" discriminator value */
> +    error_free_or_abort(&err);
> +    visitor_reset(data);
> +}
> +
> +
>  static void test_visitor_out_list(TestOutputVisitorData *data,
>                                    const void *unused)
>  {
> @@ -815,6 +857,8 @@ int main(int argc, char **argv)
>                              &out_visitor_data, test_visitor_out_struct_nested);
>      output_visitor_test_add("/visitor/output/struct-errors",
>                              &out_visitor_data, test_visitor_out_struct_errors);
> +    output_visitor_test_add("/visitor/output/partial-visit",
> +                            &out_visitor_data, test_visitor_out_partial_visit);
>      output_visitor_test_add("/visitor/output/list",
>                              &out_visitor_data, test_visitor_out_list);
>      output_visitor_test_add("/visitor/output/any",

Re: [Qemu-devel] [PATCH 2/5] tests: Enhance qobject output to cover partial visit

[Eric Blake]

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

On 07/20/2017 04:52 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> Add a test that proves (at least when run under valgrind) that
>> we are correctly handling allocated memory even when a visit
>> is aborted in the middle for whatever other reason.
>>
>> See commit f24582d "qapi: fix double free in
>> qmp_output_visitor_cleanup()" for a fix that was lacking
>> testsuite exposure prior to this patch.
>>

>> +
>> +    /*
>> +     * Abort in the middle of an alternate. Alternates can't be
>> +     * virtually visited, so we get to inline the first half of
>> +     * visit_type_UserDefAlternate().
>> +     */
> 
> Not exactly inline.  Perhaps:
> 
>        /*
>         * Abort in the middle of an alternate.  Since alternates don't
>         * support virtual visits, we perform a real one, similar to what
>         * visit_type_UserDefAlternate() would do.
>         */

Sounds reasonable, if we go with it.

> 
> Hmm, what would visit_type_UserDefAlternate() do for @uda?  Could we
> simply call it here and be done?

Sounds even better; I'll do that for v2.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

[Qemu-devel] [PATCH 3/5] qapi: Visitor documentation tweak

[Eric Blake]

Make it clear that the name parameter to visit_start_struct()
has the same semantics as for visit_start_int().

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 include/qapi/visitor.h | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index b0a048f..dc09cc7 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -60,13 +60,13 @@
  * visitors are declared here; the remaining visitors are generated in
  * qapi-visit.h.
  *
- * The @name parameter of visit_type_FOO() describes the relation
- * between this QAPI value and its parent container.  When visiting
- * the root of a tree, @name is ignored; when visiting a member of an
- * object, @name is the key associated with the value; when visiting a
- * member of a list, @name is NULL; and when visiting the member of an
- * alternate, @name should equal the name used for visiting the
- * alternate.
+ * The @name parameter of visit_type_FOO() and visit_start_OBJECT()
+ * describes the relation between this QAPI value and its parent
+ * container.  When visiting the root of a tree, @name is ignored;
+ * when visiting a member of an object, @name is the key associated
+ * with the value; when visiting a member of a list, @name is NULL;
+ * and when visiting the member of an alternate, @name should equal
+ * the name used for visiting the alternate.
  *
  * The visit_type_FOO() functions expect a non-null @obj argument;
  * they allocate *@obj during input visits, leave it unchanged on
-- 
2.9.4

Re: [Qemu-devel] [PATCH 3/5] qapi: Visitor documentation tweak

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> Make it clear that the name parameter to visit_start_struct()
> has the same semantics as for visit_start_int().
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  include/qapi/visitor.h | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
> index b0a048f..dc09cc7 100644
> --- a/include/qapi/visitor.h
> +++ b/include/qapi/visitor.h
> @@ -60,13 +60,13 @@
>   * visitors are declared here; the remaining visitors are generated in
>   * qapi-visit.h.
>   *
> - * The @name parameter of visit_type_FOO() describes the relation
> - * between this QAPI value and its parent container.  When visiting
> - * the root of a tree, @name is ignored; when visiting a member of an
> - * object, @name is the key associated with the value; when visiting a
> - * member of a list, @name is NULL; and when visiting the member of an
> - * alternate, @name should equal the name used for visiting the
> - * alternate.
> + * The @name parameter of visit_type_FOO() and visit_start_OBJECT()

Our terminology is horrible:

    JSON object / QDict   / QAPI complex type (struct or union)
    JSON array  / QList   / QAPI array or list (used interchangeably)
    JSON value  / QObject / QAPI I'm-not-even-sure-what

Because of this mess, nobody knows what you mean when you say OBJECT.

visit_start_BAR()?

> + * describes the relation between this QAPI value and its parent
> + * container.  When visiting the root of a tree, @name is ignored;
> + * when visiting a member of an object, @name is the key associated
> + * with the value; when visiting a member of a list, @name is NULL;
> + * and when visiting the member of an alternate, @name should equal
> + * the name used for visiting the alternate.
>   *
>   * The visit_type_FOO() functions expect a non-null @obj argument;
>   * they allocate *@obj during input visits, leave it unchanged on

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH 3/5] qapi: Visitor documentation tweak

[Eric Blake]

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

On 07/20/2017 05:00 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> Make it clear that the name parameter to visit_start_struct()
>> has the same semantics as for visit_start_int().
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---

>> + * The @name parameter of visit_type_FOO() and visit_start_OBJECT()
> 
> Our terminology is horrible:
> 
>     JSON object / QDict   / QAPI complex type (struct or union)
>     JSON array  / QList   / QAPI array or list (used interchangeably)
>     JSON value  / QObject / QAPI I'm-not-even-sure-what
> 
> Because of this mess, nobody knows what you mean when you say OBJECT.
> 
> visit_start_BAR()?

Works for me.

> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Since you suggested it, I'll keep your R-b when I send v2.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

[Qemu-devel] [PATCH 4/5] qtest: Avoid passing raw strings through hmp()

[Eric Blake]

The next patch will add __attribute__((__format__)) to hmp(), which
in turn causes gcc to warn about non-literal format strings.  Rather
than risk an arbitrary string containing % being mis-handled, always
pass variable strings along with a %s format.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 tests/test-hmp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/test-hmp.c b/tests/test-hmp.c
index d77b3c8..0af0664 100644
--- a/tests/test-hmp.c
+++ b/tests/test-hmp.c
@@ -80,7 +80,7 @@ static void test_commands(void)
         if (verbose) {
             fprintf(stderr, "\t%s\n", hmp_cmds[i]);
         }
-        response = hmp(hmp_cmds[i]);
+        response = hmp("%s", hmp_cmds[i]);
         g_free(response);
     }

@@ -103,7 +103,7 @@ static void test_info_commands(void)
         if (verbose) {
             fprintf(stderr, "\t%s\n", info);
         }
-        resp = hmp(info);
+        resp = hmp("%s", info);
         g_free(resp);
         /* And move forward to the next line */
         info = strchr(endp + 1, '\n');
-- 
2.9.4

Re: [Qemu-devel] [PATCH 4/5] qtest: Avoid passing raw strings through hmp()

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> The next patch will add __attribute__((__format__)) to hmp(), which
> in turn causes gcc to warn about non-literal format strings.  Rather
> than risk an arbitrary string containing % being mis-handled, always
> pass variable strings along with a %s format.

More importantly (for me), "%s" makes correctness locally obvious.
Before the patch, I have to prove the argument can't contain '%'.

> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  tests/test-hmp.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tests/test-hmp.c b/tests/test-hmp.c
> index d77b3c8..0af0664 100644
> --- a/tests/test-hmp.c
> +++ b/tests/test-hmp.c
> @@ -80,7 +80,7 @@ static void test_commands(void)
>          if (verbose) {
>              fprintf(stderr, "\t%s\n", hmp_cmds[i]);
>          }
> -        response = hmp(hmp_cmds[i]);
> +        response = hmp("%s", hmp_cmds[i]);
>          g_free(response);
>      }
>
> @@ -103,7 +103,7 @@ static void test_info_commands(void)
>          if (verbose) {
>              fprintf(stderr, "\t%s\n", info);
>          }
> -        resp = hmp(info);
> +        resp = hmp("%s", info);
>          g_free(resp);
>          /* And move forward to the next line */
>          info = strchr(endp + 1, '\n');

Reviewed-by: Markus Armbruster <armbru@redhat.com>

[Qemu-devel] [PATCH 5/5] qtest: Document calling conventions

[Eric Blake]

We have two flavors of vararg usage in qtest; make it clear that
qmp() has different semantics than hmp(), and let the compiler
enforce that hmp() is used correctly. Since qmp() only accepts
a subset of printf flags (namely, those that our JSON parser
understands), I figured that it is probably not worth adding a
format attribute to qmp() at this time.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 tests/libqtest.h | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/tests/libqtest.h b/tests/libqtest.h
index 38bc1e9..762ed13 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -50,7 +50,8 @@ void qtest_quit(QTestState *s);
 /**
  * qtest_qmp_discard_response:
  * @s: #QTestState instance to operate on.
- * @fmt...: QMP message to send to qemu
+ * @fmt...: QMP message to send to qemu; only recognizes formats
+ * understood by json-lexer.c
  *
  * Sends a QMP message to QEMU and consumes the response.
  */
@@ -59,7 +60,8 @@ void qtest_qmp_discard_response(QTestState *s, const char *fmt, ...);
 /**
  * qtest_qmp:
  * @s: #QTestState instance to operate on.
- * @fmt...: QMP message to send to qemu
+ * @fmt...: QMP message to send to qemu; only recognizes formats
+ * understood by json-lexer.c
  *
  * Sends a QMP message to QEMU and returns the response.
  */
@@ -134,14 +136,14 @@ QDict *qtest_qmp_eventwait_ref(QTestState *s, const char *event);
 /**
  * qtest_hmp:
  * @s: #QTestState instance to operate on.
- * @fmt...: HMP command to send to QEMU
+ * @fmt...: HMP command to send to QEMU, passed through sprintf()
  *
  * Send HMP command to QEMU via QMP's human-monitor-command.
  * QMP events are discarded.
  *
  * Returns: the command's output.  The caller should g_free() it.
  */
-char *qtest_hmp(QTestState *s, const char *fmt, ...);
+char *qtest_hmp(QTestState *s, const char *fmt, ...) GCC_FMT_ATTR(2, 3);

 /**
  * qtest_hmpv:
@@ -535,7 +537,8 @@ static inline void qtest_end(void)

 /**
  * qmp:
- * @fmt...: QMP message to send to qemu
+ * @fmt...: QMP message to send to qemu; only recognizes formats
+ * understood by json-lexer.c
  *
  * Sends a QMP message to QEMU and returns the response.
  */
@@ -543,7 +546,8 @@ QDict *qmp(const char *fmt, ...);

 /**
  * qmp_async:
- * @fmt...: QMP message to send to qemu
+ * @fmt...: QMP message to send to qemu; only recognizes formats
+ * understood by json-lexer.c
  *
  * Sends a QMP message to QEMU and leaves the response in the stream.
  */
@@ -551,7 +555,8 @@ void qmp_async(const char *fmt, ...);

 /**
  * qmp_discard_response:
- * @fmt...: QMP message to send to qemu
+ * @fmt...: QMP message to send to qemu; only recognizes formats
+ * understood by json-lexer.c
  *
  * Sends a QMP message to QEMU and consumes the response.
  */
@@ -592,13 +597,13 @@ static inline QDict *qmp_eventwait_ref(const char *event)

 /**
  * hmp:
- * @fmt...: HMP command to send to QEMU
+ * @fmt...: HMP command to send to QEMU, passed through printf()
  *
  * Send HMP command to QEMU via QMP's human-monitor-command.
  *
  * Returns: the command's output.  The caller should g_free() it.
  */
-char *hmp(const char *fmt, ...);
+char *hmp(const char *fmt, ...) GCC_FMT_ATTR(1, 2);

 /**
  * get_irq:
-- 
2.9.4

Re: [Qemu-devel] [PATCH 5/5] qtest: Document calling conventions

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> We have two flavors of vararg usage in qtest; make it clear that
> qmp() has different semantics than hmp(), and let the compiler
> enforce that hmp() is used correctly. Since qmp() only accepts
> a subset of printf flags (namely, those that our JSON parser
> understands), I figured that it is probably not worth adding a
> format attribute to qmp() at this time.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  tests/libqtest.h | 23 ++++++++++++++---------
>  1 file changed, 14 insertions(+), 9 deletions(-)
>
> diff --git a/tests/libqtest.h b/tests/libqtest.h
> index 38bc1e9..762ed13 100644
> --- a/tests/libqtest.h
> +++ b/tests/libqtest.h
> @@ -50,7 +50,8 @@ void qtest_quit(QTestState *s);
>  /**
>   * qtest_qmp_discard_response:
>   * @s: #QTestState instance to operate on.
> - * @fmt...: QMP message to send to qemu
> + * @fmt...: QMP message to send to qemu; only recognizes formats
> + * understood by json-lexer.c
>   *
>   * Sends a QMP message to QEMU and consumes the response.
>   */

These formats are chosen so that gcc can help us check them.  Please add
GCC_FMT_ATTR().  Precedence: qobject_from_jsonf().

Where are the "formats understood by json-lexer.c" documented?

More of the same below.

> @@ -59,7 +60,8 @@ void qtest_qmp_discard_response(QTestState *s, const char *fmt, ...);
>  /**
>   * qtest_qmp:
>   * @s: #QTestState instance to operate on.
> - * @fmt...: QMP message to send to qemu
> + * @fmt...: QMP message to send to qemu; only recognizes formats
> + * understood by json-lexer.c
>   *
>   * Sends a QMP message to QEMU and returns the response.
>   */
> @@ -134,14 +136,14 @@ QDict *qtest_qmp_eventwait_ref(QTestState *s, const char *event);
>  /**
>   * qtest_hmp:
>   * @s: #QTestState instance to operate on.
> - * @fmt...: HMP command to send to QEMU
> + * @fmt...: HMP command to send to QEMU, passed through sprintf()

Not actually through sprintf(), but I get what you mean.  I like to
document such things as "Format arguments like vsprintf()."

>   *
>   * Send HMP command to QEMU via QMP's human-monitor-command.
>   * QMP events are discarded.
>   *
>   * Returns: the command's output.  The caller should g_free() it.
>   */
> -char *qtest_hmp(QTestState *s, const char *fmt, ...);
> +char *qtest_hmp(QTestState *s, const char *fmt, ...) GCC_FMT_ATTR(2, 3);
>
>  /**
>   * qtest_hmpv:
> @@ -535,7 +537,8 @@ static inline void qtest_end(void)
>
>  /**
>   * qmp:
> - * @fmt...: QMP message to send to qemu
> + * @fmt...: QMP message to send to qemu; only recognizes formats
> + * understood by json-lexer.c
>   *
>   * Sends a QMP message to QEMU and returns the response.
>   */
> @@ -543,7 +546,8 @@ QDict *qmp(const char *fmt, ...);
>
>  /**
>   * qmp_async:
> - * @fmt...: QMP message to send to qemu
> + * @fmt...: QMP message to send to qemu; only recognizes formats
> + * understood by json-lexer.c
>   *
>   * Sends a QMP message to QEMU and leaves the response in the stream.
>   */
> @@ -551,7 +555,8 @@ void qmp_async(const char *fmt, ...);
>
>  /**
>   * qmp_discard_response:
> - * @fmt...: QMP message to send to qemu
> + * @fmt...: QMP message to send to qemu; only recognizes formats
> + * understood by json-lexer.c
>   *
>   * Sends a QMP message to QEMU and consumes the response.
>   */
> @@ -592,13 +597,13 @@ static inline QDict *qmp_eventwait_ref(const char *event)
>
>  /**
>   * hmp:
> - * @fmt...: HMP command to send to QEMU
> + * @fmt...: HMP command to send to QEMU, passed through printf()

Here, you claim printf().  Typo?

>   *
>   * Send HMP command to QEMU via QMP's human-monitor-command.
>   *
>   * Returns: the command's output.  The caller should g_free() it.
>   */
> -char *hmp(const char *fmt, ...);
> +char *hmp(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
>
>  /**
>   * get_irq:

Re: [Qemu-devel] [PATCH 5/5] qtest: Document calling conventions

[Eric Blake]

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

On 07/20/2017 05:10 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> We have two flavors of vararg usage in qtest; make it clear that
>> qmp() has different semantics than hmp(), and let the compiler
>> enforce that hmp() is used correctly. Since qmp() only accepts
>> a subset of printf flags (namely, those that our JSON parser
>> understands), I figured that it is probably not worth adding a
>> format attribute to qmp() at this time.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>>  tests/libqtest.h | 23 ++++++++++++++---------
>>  1 file changed, 14 insertions(+), 9 deletions(-)
>>
>> diff --git a/tests/libqtest.h b/tests/libqtest.h
>> index 38bc1e9..762ed13 100644
>> --- a/tests/libqtest.h
>> +++ b/tests/libqtest.h
>> @@ -50,7 +50,8 @@ void qtest_quit(QTestState *s);
>>  /**
>>   * qtest_qmp_discard_response:
>>   * @s: #QTestState instance to operate on.
>> - * @fmt...: QMP message to send to qemu
>> + * @fmt...: QMP message to send to qemu; only recognizes formats
>> + * understood by json-lexer.c
>>   *
>>   * Sends a QMP message to QEMU and consumes the response.
>>   */
> 
> These formats are chosen so that gcc can help us check them.  Please add
> GCC_FMT_ATTR().  Precedence: qobject_from_jsonf().

Will do.  (This patch was originally part of my larger series trying to
get rid of qobject_from_jsonf() - I may still succeed at that, but
separating the easy stuff from the controversial means that the easy
stuff needs tweaking).

> 
> Where are the "formats understood by json-lexer.c" documented?

Near the top of qobject/json-lexer.c:

 * Extension for vararg handling in JSON construction:
 *
 * %((l|ll|I64)?d|[ipsf])

Note that %i differs from %d (the former produces true/false, while the
latter produces "42" and friends - but it happens to "work" with gcc's
-Wformat checking, thanks to var-arg type promotion rules).

I could just spell it out directly (in fact, in the above-mentioned
larger series, I still kept qtest_qmp() able to understand %s, but
dropped all the other formats like %ld and %p).

>> + * @fmt...: HMP command to send to QEMU, passed through sprintf()
> 
> Not actually through sprintf(), but I get what you mean.  I like to
> document such things as "Format arguments like vsprintf()."

Works for me.

>> @@ -592,13 +597,13 @@ static inline QDict *qmp_eventwait_ref(const char *event)
>>
>>  /**
>>   * hmp:
>> - * @fmt...: HMP command to send to QEMU
>> + * @fmt...: HMP command to send to QEMU, passed through printf()
> 
> Here, you claim printf().  Typo?

Or inconsistent copy-and-paste.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH 5/5] qtest: Document calling conventions

[Eric Blake]

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

On 07/20/2017 03:37 PM, Eric Blake wrote:
>>> + * @fmt...: QMP message to send to qemu; only recognizes formats
>>> + * understood by json-lexer.c
>>>   *
>>>   * Sends a QMP message to QEMU and consumes the response.
>>>   */
>>
>> These formats are chosen so that gcc can help us check them.  Please add
>> GCC_FMT_ATTR().  Precedence: qobject_from_jsonf().
> 
> Will do.  (This patch was originally part of my larger series trying to
> get rid of qobject_from_jsonf() - I may still succeed at that, but
> separating the easy stuff from the controversial means that the easy
> stuff needs tweaking).

Bleargh.  It's not that simple.

With printf-style, hmp("literal") and hmp("%s", "literal") produce the
same results.

But with json-lexer style, %s MODIFIES its input.
The original qmp("{'execute':\"foo\"}") sends a JSON object
 {'execute':"foo"}
but counterpart qmp("%s", "{'execute':'foo'}") sends a JSON string with
added \ escaping:
 "{'execute':\"foo\"}"

So adding the format immediately causes lots of warnings, such as:

  CC      tests/vhost-user-test.o
tests/vhost-user-test.c: In function ‘test_migrate’:
tests/vhost-user-test.c:668:5: error: format not a string literal and no
format arguments [-Werror=format-security]
     rsp = qmp(cmd);
     ^~~

  CC      tests/boot-order-test.o
tests/boot-order-test.c: In function ‘test_a_boot_order’:
tests/boot-order-test.c:46:26: error: zero-length gnu_printf format
string [-Werror=format-zero-length]
     qmp_discard_response("");   /* HACK: wait for event */
                          ^~

but we CAN'T rewrite those to qmp("%s", command).

Now you can sort-of understand why my larger series wanted to get rid of
qobject_from_jsonf(), since our use of GCC_FMT_ATTR() there is a lie?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH 5/5] qtest: Document calling conventions

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 07/20/2017 03:37 PM, Eric Blake wrote:
>>>> + * @fmt...: QMP message to send to qemu; only recognizes formats
>>>> + * understood by json-lexer.c
>>>>   *
>>>>   * Sends a QMP message to QEMU and consumes the response.
>>>>   */
>>>
>>> These formats are chosen so that gcc can help us check them.  Please add
>>> GCC_FMT_ATTR().  Precedence: qobject_from_jsonf().
>> 
>> Will do.  (This patch was originally part of my larger series trying to
>> get rid of qobject_from_jsonf() - I may still succeed at that, but
>> separating the easy stuff from the controversial means that the easy
>> stuff needs tweaking).
>
> Bleargh.  It's not that simple.
>
> With printf-style, hmp("literal") and hmp("%s", "literal") produce the
> same results.
>
> But with json-lexer style, %s MODIFIES its input.

Your assertion "MODIFIES its input" confused me briefly, because I read
it as "side effect on the argument string".  That would be bonkers.
What you mean is it doesn't emit the argument string unmodified.

> The original qmp("{'execute':\"foo\"}") sends a JSON object
>  {'execute':"foo"}
> but counterpart qmp("%s", "{'execute':'foo'}") sends a JSON string with
> added \ escaping:
>  "{'execute':\"foo\"}"
>
> So adding the format immediately causes lots of warnings, such as:
>
>   CC      tests/vhost-user-test.o
> tests/vhost-user-test.c: In function ‘test_migrate’:
> tests/vhost-user-test.c:668:5: error: format not a string literal and no
> format arguments [-Werror=format-security]
>      rsp = qmp(cmd);
>      ^~~

    cmd = g_strdup_printf("{ 'execute': 'migrate',"
                          "'arguments': { 'uri': '%s' } }",
                          uri);
    rsp = qmp(cmd);
    g_free(cmd);
    g_assert(qdict_haskey(rsp, "return"));
    QDECREF(rsp);

For this to work, @uri must not contain funny characters.

Shouldn't we simply use the built-in quoting here?

    rsp = qmp("{ 'execute': 'migrate', 'arguments': { 'uri': %s } }", uri);
    g_assert(qdict_haskey(rsp, "return"));
    QDECREF(rsp);

>
>   CC      tests/boot-order-test.o
> tests/boot-order-test.c: In function ‘test_a_boot_order’:
> tests/boot-order-test.c:46:26: error: zero-length gnu_printf format
> string [-Werror=format-zero-length]
>      qmp_discard_response("");   /* HACK: wait for event */
>                           ^~

Should use qmp_eventwait().  Doesn't because it predates it.

> but we CAN'T rewrite those to qmp("%s", command).

Got more troublemakers?

> Now you can sort-of understand why my larger series wanted to get rid of
> qobject_from_jsonf(), since our use of GCC_FMT_ATTR() there is a lie?

I don't think it's a lie.  qobject_from_jsonf() satisfies the attribute
format(printf, ...) contract: it fetches varargs exactly like printf().
What it does with them is of no concern to this attribute.

Re: [Qemu-devel] [PATCH 5/5] qtest: Document calling conventions

[Eric Blake]

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

On 07/21/2017 01:42 AM, Markus Armbruster wrote:
>> But with json-lexer style, %s MODIFIES its input.
> 
> Your assertion "MODIFIES its input" confused me briefly, because I read
> it as "side effect on the argument string".  That would be bonkers.
> What you mean is it doesn't emit the argument string unmodified.

Yes. I'm not sure how I could have worded that better; maybe "does not
do a straight passthrough of its input"

>> So adding the format immediately causes lots of warnings, such as:
>>
>>   CC      tests/vhost-user-test.o
>> tests/vhost-user-test.c: In function ‘test_migrate’:
>> tests/vhost-user-test.c:668:5: error: format not a string literal and no
>> format arguments [-Werror=format-security]
>>      rsp = qmp(cmd);
>>      ^~~
> 
>     cmd = g_strdup_printf("{ 'execute': 'migrate',"
>                           "'arguments': { 'uri': '%s' } }",
>                           uri);
>     rsp = qmp(cmd);
>     g_free(cmd);
>     g_assert(qdict_haskey(rsp, "return"));
>     QDECREF(rsp);
> 
> For this to work, @uri must not contain funny characters.
> 
> Shouldn't we simply use the built-in quoting here?

We can - but there are a lot of tests that have to be updated.

> 
>     rsp = qmp("{ 'execute': 'migrate', 'arguments': { 'uri': %s } }", uri);
>     g_assert(qdict_haskey(rsp, "return"));
>     QDECREF(rsp);
> 
>>
>>   CC      tests/boot-order-test.o
>> tests/boot-order-test.c: In function ‘test_a_boot_order’:
>> tests/boot-order-test.c:46:26: error: zero-length gnu_printf format
>> string [-Werror=format-zero-length]
>>      qmp_discard_response("");   /* HACK: wait for event */
>>                           ^~
> 
> Should use qmp_eventwait().  Doesn't because it predates it.

We can - but there are a lot of tests that have to be updated.

> 
>> but we CAN'T rewrite those to qmp("%s", command).
> 
> Got more troublemakers?

When I worked on getting rid of qobject_from_jsonf(), I was able to
reduce the tests down to JUST using %s, which I then handled locally in
qmp() rather than relying on qobject_from_jsonf().  Going the opposite
direction and more fully relying on qobject_from_jsonf() by fixing
multiple tests that were using older idioms is also an approach (in the
opposite direction I originally took) - but the more work it is, the
less likely that this patch is 2.10 material.

> 
>> Now you can sort-of understand why my larger series wanted to get rid of
>> qobject_from_jsonf(), since our use of GCC_FMT_ATTR() there is a lie?
> 
> I don't think it's a lie.  qobject_from_jsonf() satisfies the attribute
> format(printf, ...) contract: it fetches varargs exactly like printf().
> What it does with them is of no concern to this attribute.

I still find it odd that you can't safely turn foo(var) into foo("%s", var).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH 5/5] qtest: Document calling conventions

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 07/21/2017 01:42 AM, Markus Armbruster wrote:
>>> But with json-lexer style, %s MODIFIES its input.
>> 
>> Your assertion "MODIFIES its input" confused me briefly, because I read
>> it as "side effect on the argument string".  That would be bonkers.
>> What you mean is it doesn't emit the argument string unmodified.
>
> Yes. I'm not sure how I could have worded that better; maybe "does not
> do a straight passthrough of its input"
>
>>> So adding the format immediately causes lots of warnings, such as:
>>>
>>>   CC      tests/vhost-user-test.o
>>> tests/vhost-user-test.c: In function ‘test_migrate’:
>>> tests/vhost-user-test.c:668:5: error: format not a string literal and no
>>> format arguments [-Werror=format-security]
>>>      rsp = qmp(cmd);
>>>      ^~~
>> 
>>     cmd = g_strdup_printf("{ 'execute': 'migrate',"
>>                           "'arguments': { 'uri': '%s' } }",
>>                           uri);
>>     rsp = qmp(cmd);
>>     g_free(cmd);
>>     g_assert(qdict_haskey(rsp, "return"));
>>     QDECREF(rsp);
>> 
>> For this to work, @uri must not contain funny characters.
>> 
>> Shouldn't we simply use the built-in quoting here?
>
> We can - but there are a lot of tests that have to be updated.

Not that many, see "[PATCH 0/9] tests: Clean up around qmp() and hmp()".
Its PATCH 4/9 makes the case for built-in quoting:

    When you build QMP input manually like this

        cmd = g_strdup_printf("{ 'execute': 'migrate',"
                              "'arguments': { 'uri': '%s' } }",
                              uri);
        rsp = qmp(cmd);
        g_free(cmd);

    you're responsible for escaping the interpolated values for JSON.  Not
    done here, and therefore works only for sufficiently nice @uri.  For
    instance, if @uri contained a single "'", qobject_from_jsonv() would
    fail, qmp_fd_sendv() would misinterpret the failure as empty input and
    do nothing, and the test would hang waiting for a response that never
    comes.

    Leaving interpolation into JSON to qmp() is more robust:

        rsp = qmp("{ 'execute': 'migrate', 'arguments': { 'uri': %s } }", uri);

    It's also more concise.

In short, using printf() & similar to format JSON is a JSON injection
vulnerability waiting to happen.  Special case: g_strdup_printf() to
format input for qobject_from_jsonf() & friends.  Leaving the job to
qobject_from_jsonf() is more robust.

>> 
>>     rsp = qmp("{ 'execute': 'migrate', 'arguments': { 'uri': %s } }", uri);
>>     g_assert(qdict_haskey(rsp, "return"));
>>     QDECREF(rsp);
>> 
>>>
>>>   CC      tests/boot-order-test.o
>>> tests/boot-order-test.c: In function ‘test_a_boot_order’:
>>> tests/boot-order-test.c:46:26: error: zero-length gnu_printf format
>>> string [-Werror=format-zero-length]
>>>      qmp_discard_response("");   /* HACK: wait for event */
>>>                           ^~
>> 
>> Should use qmp_eventwait().  Doesn't because it predates it.
>
> We can - but there are a lot of tests that have to be updated.

Also not that many, see same series.

>>> but we CAN'T rewrite those to qmp("%s", command).
>> 
>> Got more troublemakers?
>
> When I worked on getting rid of qobject_from_jsonf(), I was able to
> reduce the tests down to JUST using %s, which I then handled locally in
> qmp() rather than relying on qobject_from_jsonf().  Going the opposite
> direction and more fully relying on qobject_from_jsonf() by fixing
> multiple tests that were using older idioms is also an approach (in the
> opposite direction I originally took) - but the more work it is, the
> less likely that this patch is 2.10 material.

No need to worry about 2.10, I think.

>>> Now you can sort-of understand why my larger series wanted to get rid of
>>> qobject_from_jsonf(), since our use of GCC_FMT_ATTR() there is a lie?
>> 
>> I don't think it's a lie.  qobject_from_jsonf() satisfies the attribute
>> format(printf, ...) contract: it fetches varargs exactly like printf().
>> What it does with them is of no concern to this attribute.
>
> I still find it odd that you can't safely turn foo(var) into foo("%s", var).

For me, the protection against JSON injection bugs is well worth it.

Re: [Qemu-devel] [PATCH 0/5] random qapi cleanups

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> Trying to flush some of the random patches I have lying around
> locally.  Some of these may have been posted to the list before,
> perhaps as part of larger series, but I didn't bother to research
> which parts came from where.
>
> I'm not sure if this is in time for softfreeze; and if not, I'm
> not sure how much is appropriate during freeze vs. being deferred
> to 2.11.

A quick peek makes me think this series could in early during freeze,
because it consists of documentation fixes, test enhancements and really
simple bug fixes.