[Qemu-devel] [PATCH 0/2] block/qapi: trivial fixes

[Qemu-devel] [PATCH 0/2] block/qapi: trivial fixes

[Peter Xu]

One is to use literal printf format when possible.

One is to fix an unbounded usage of stack.

Peter Xu (2):
  block/qapi: make two printf() formats literal
  block/qapi: fix unbounded stack for dump_qdict

 block/qapi.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH 1/2] block/qapi: make two printf() formats literal

[Peter Xu]

Fix two places to use literal printf format when possible.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 block/qapi.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/block/qapi.c b/block/qapi.c
index db2d3fb..c4c2115 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -619,9 +619,8 @@ static void dump_qlist(fprintf_function func_fprintf, void *f, int indentation,
     for (entry = qlist_first(list); entry; entry = qlist_next(entry), i++) {
         QType type = qobject_type(entry->value);
         bool composite = (type == QTYPE_QDICT || type == QTYPE_QLIST);
-        const char *format = composite ? "%*s[%i]:\n" : "%*s[%i]: ";
-
-        func_fprintf(f, format, indentation * 4, "", i);
+        func_fprintf(f, "%*s[%i]:%c", indentation * 4, "", i,
+                     composite ? '\n' : ' ');
         dump_qobject(func_fprintf, f, indentation + 1, entry->value);
         if (!composite) {
             func_fprintf(f, "\n");
@@ -637,7 +636,6 @@ static void dump_qdict(fprintf_function func_fprintf, void *f, int indentation,
     for (entry = qdict_first(dict); entry; entry = qdict_next(dict, entry)) {
         QType type = qobject_type(entry->value);
         bool composite = (type == QTYPE_QDICT || type == QTYPE_QLIST);
-        const char *format = composite ? "%*s%s:\n" : "%*s%s: ";
         char key[strlen(entry->key) + 1];
         int i;
 
@@ -646,8 +644,8 @@ static void dump_qdict(fprintf_function func_fprintf, void *f, int indentation,
             key[i] = entry->key[i] == '-' ? ' ' : entry->key[i];
         }
         key[i] = 0;
-
-        func_fprintf(f, format, indentation * 4, "", key);
+        func_fprintf(f, "%*s%s:%c", indentation * 4, "", key,
+                     composite ? '\n' : ' ');
         dump_qobject(func_fprintf, f, indentation + 1, entry->value);
         if (!composite) {
             func_fprintf(f, "\n");
-- 
2.4.3

[Qemu-devel] [PATCH 2/2] block/qapi: fix unbounded stack for dump_qdict

[Peter Xu]

Using heap instead of stack for better safety.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 block/qapi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/qapi.c b/block/qapi.c
index c4c2115..b798e35 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -636,9 +636,8 @@ static void dump_qdict(fprintf_function func_fprintf, void *f, int indentation,
     for (entry = qdict_first(dict); entry; entry = qdict_next(dict, entry)) {
         QType type = qobject_type(entry->value);
         bool composite = (type == QTYPE_QDICT || type == QTYPE_QLIST);
-        char key[strlen(entry->key) + 1];
+        char *key = g_malloc(strlen(entry->key) + 1);
         int i;
-
         /* replace dashes with spaces in key (variable) names */
         for (i = 0; entry->key[i]; i++) {
             key[i] = entry->key[i] == '-' ? ' ' : entry->key[i];
@@ -650,6 +649,7 @@ static void dump_qdict(fprintf_function func_fprintf, void *f, int indentation,
         if (!composite) {
             func_fprintf(f, "\n");
         }
+        g_free(key);
     }
 }
 
-- 
2.4.3

Re: [Qemu-devel] [PATCH 1/2] block/qapi: make two printf() formats literal

[Eric Blake]

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

On 03/08/2016 10:56 PM, Peter Xu wrote:
> Fix two places to use literal printf format when possible.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  block/qapi.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/block/qapi.c b/block/qapi.c
> index db2d3fb..c4c2115 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -619,9 +619,8 @@ static void dump_qlist(fprintf_function func_fprintf, void *f, int indentation,
>      for (entry = qlist_first(list); entry; entry = qlist_next(entry), i++) {
>          QType type = qobject_type(entry->value);
>          bool composite = (type == QTYPE_QDICT || type == QTYPE_QLIST);
> -        const char *format = composite ? "%*s[%i]:\n" : "%*s[%i]: ";
> -
> -        func_fprintf(f, format, indentation * 4, "", i);
> +        func_fprintf(f, "%*s[%i]:%c", indentation * 4, "", i,
> +                     composite ? '\n' : ' ');

[The nerd in me wants to point out that you could avoid the ternary by
writing '"\n "[composite]', but that's too ugly to use outside of IOCCC
submissions, and I wouldn't be surprised if it (rightfully) triggers
clang warnings]

-- 
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 2/2] block/qapi: fix unbounded stack for dump_qdict

[Eric Blake]

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

On 03/08/2016 10:56 PM, Peter Xu wrote:
> Using heap instead of stack for better safety.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  block/qapi.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/block/qapi.c b/block/qapi.c
> index c4c2115..b798e35 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -636,9 +636,8 @@ static void dump_qdict(fprintf_function func_fprintf, void *f, int indentation,
>      for (entry = qdict_first(dict); entry; entry = qdict_next(dict, entry)) {
>          QType type = qobject_type(entry->value);
>          bool composite = (type == QTYPE_QDICT || type == QTYPE_QLIST);
> -        char key[strlen(entry->key) + 1];
> +        char *key = g_malloc(strlen(entry->key) + 1);
>          int i;
> -
>          /* replace dashes with spaces in key (variable) names */
>          for (i = 0; entry->key[i]; i++) {
>              key[i] = entry->key[i] == '-' ? ' ' : entry->key[i];
> @@ -650,6 +649,7 @@ static void dump_qdict(fprintf_function func_fprintf, void *f, int indentation,
>          if (!composite) {
>              func_fprintf(f, "\n");
>          }
> +        g_free(key);
>      }
>  }
>  
> 

-- 
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 1/2] block/qapi: make two printf() formats literal

[Peter Xu]

On Wed, Mar 09, 2016 at 03:14:03PM -0700, Eric Blake wrote:
> > +        func_fprintf(f, "%*s[%i]:%c", indentation * 4, "", i,
> > +                     composite ? '\n' : ' ');
> 
> [The nerd in me wants to point out that you could avoid the ternary by
> writing '"\n "[composite]', but that's too ugly to use outside of IOCCC
> submissions, and I wouldn't be surprised if it (rightfully) triggers
> clang warnings]

Do you mean something like:

int i = 0;
printf("%c", '"\n "[i]');

Is this a grammar btw?

Peter

Re: [Qemu-devel] [PATCH 1/2] block/qapi: make two printf() formats literal

[Eric Blake]

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

On 03/09/2016 06:46 PM, Peter Xu wrote:
> On Wed, Mar 09, 2016 at 03:14:03PM -0700, Eric Blake wrote:
>>> +        func_fprintf(f, "%*s[%i]:%c", indentation * 4, "", i,
>>> +                     composite ? '\n' : ' ');
>>
>> [The nerd in me wants to point out that you could avoid the ternary by
>> writing '"\n "[composite]', but that's too ugly to use outside of IOCCC
>> submissions, and I wouldn't be surprised if it (rightfully) triggers
>> clang warnings]
> 
> Do you mean something like:
> 
> int i = 0;
> printf("%c", '"\n "[i]');

You mean:

printf("%c", "\n "[i]);

(no '').  But with your declaration of 'i' as int, it is only defined if
i <= 2 (whereas in my example above, "\n "[composite] is always defined
because composite is bool rather than int).

> 
> Is this a grammar btw?

Yes, C has an ugly grammar, because [] is just syntactic sugar for
deferencing pointer addition with nicer operator precedence.  Quoting
C99 6.5.2.1:

"The definition of the subscript operator [] is that E1[E2] is identical
to (*((E1)+(E2))).  Because of the conversion rules that apply to the
binary + operator, if E1 is an array object (equivalently, a pointer to
the initial element of an array object) and E2 is an integer, E1[E2]
designates the E2-th element of E1 (counting from zero)."

And a string literal is just a fancy way of writing the address of an
array of characters (where the address is chosen by the compiler).

Thus, it IS valid to dereference the addition of an integer offset with
the address implied by a string literal in order to obtain a character
within the string.  And since the [] operator is commutative (even
though no one in their right mind commutes the operands), you can also
write the even-uglier:

composite["\n "]

But now we've gone far astray from the original patch review :)

-- 
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 1/2] block/qapi: make two printf() formats literal

[Peter Xu]

On Mon, Mar 21, 2016 at 03:14:48PM -0600, Eric Blake wrote:
> On 03/09/2016 06:46 PM, Peter Xu wrote:
> > 
> > Is this a grammar btw?
> 
> Yes, C has an ugly grammar, because [] is just syntactic sugar for
> deferencing pointer addition with nicer operator precedence.  Quoting
> C99 6.5.2.1:
> 
> "The definition of the subscript operator [] is that E1[E2] is identical
> to (*((E1)+(E2))).  Because of the conversion rules that apply to the
> binary + operator, if E1 is an array object (equivalently, a pointer to
> the initial element of an array object) and E2 is an integer, E1[E2]
> designates the E2-th element of E1 (counting from zero)."
> 
> And a string literal is just a fancy way of writing the address of an
> array of characters (where the address is chosen by the compiler).
> 
> Thus, it IS valid to dereference the addition of an integer offset with
> the address implied by a string literal in order to obtain a character
> within the string.  And since the [] operator is commutative (even
> though no one in their right mind commutes the operands), you can also
> write the even-uglier:
> 
> composite["\n "]
> 
> But now we've gone far astray from the original patch review :)

Interesting thing to know.  Thanks. :)

-- peterx

Re: [Qemu-devel] [PATCH 1/2] block/qapi: make two printf() formats literal

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> Fix two places to use literal printf format when possible.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

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

Re: [Qemu-devel] [PATCH 2/2] block/qapi: fix unbounded stack for dump_qdict

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> Using heap instead of stack for better safety.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  block/qapi.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/block/qapi.c b/block/qapi.c
> index c4c2115..b798e35 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -636,9 +636,8 @@ static void dump_qdict(fprintf_function func_fprintf, void *f, int indentation,
>      for (entry = qdict_first(dict); entry; entry = qdict_next(dict, entry)) {
>          QType type = qobject_type(entry->value);
>          bool composite = (type == QTYPE_QDICT || type == QTYPE_QLIST);
> -        char key[strlen(entry->key) + 1];
> +        char *key = g_malloc(strlen(entry->key) + 1);
>          int i;
> -

Unwanted whitespace change.

>          /* replace dashes with spaces in key (variable) names */
>          for (i = 0; entry->key[i]; i++) {
>              key[i] = entry->key[i] == '-' ? ' ' : entry->key[i];
> @@ -650,6 +649,7 @@ static void dump_qdict(fprintf_function func_fprintf, void *f, int indentation,
>          if (!composite) {
>              func_fprintf(f, "\n");
>          }
> +        g_free(key);
>      }
>  }

With the unwanted whitespace change backed out:
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH 0/2] block/qapi: trivial fixes

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> One is to use literal printf format when possible.
>
> One is to fix an unbounded usage of stack.

I lack the time to take this through my tree before my Easter vacation.
Kevin, can you stick it into your next pull request for me?  Note
trivial fixup on PATCH 2.

Re: [Qemu-devel] [PATCH 0/2] block/qapi: trivial fixes

[Kevin Wolf]

Am 22.03.2016 um 16:50 hat Markus Armbruster geschrieben:
> Peter Xu <peterx@redhat.com> writes:
> 
> > One is to use literal printf format when possible.
> >
> > One is to fix an unbounded usage of stack.
> 
> I lack the time to take this through my tree before my Easter vacation.
> Kevin, can you stick it into your next pull request for me?  Note
> trivial fixup on PATCH 2.

Thanks, applied to the block branch.

Kevin