[Qemu-devel] [PATCH v4 0/2] Fix tests on recent gcc

[Qemu-devel] [PATCH v4 0/2] Fix tests on recent gcc

[Juan Quintela]

Hi

v4: really sent the patches for v3, not for v2.  Sorry folks

[v3]
- change assert from position

Please review.

[v2]

- move all uses of -1 for memtest.
- remove docs about the feature.
- use size_t for the second patch (cedric)

Please, review.


[v1]

On Fedora 26 (gcc-7.1.1 and glib2 2.52.3) tests don't compile.

In file included from /usr/include/string.h:639:0,
                 from /mnt/kvm/qemu/cleanup/include/qemu/osdep.h:69,
                 from /mnt/kvm/qemu/cleanup/tests/test-iov.c:1:
In function ‘memcpy’,
    inlined from ‘iov_from_buf’ at /mnt/kvm/qemu/cleanup/include/qemu/iov.h:51:9,
    inlined from ‘test_to_from_buf_1’ at /mnt/kvm/qemu/cleanup/tests/test-iov.c:88:14,
    inlined from ‘test_to_from_buf’ at /mnt/kvm/qemu/cleanup/tests/test-iov.c:144:9:
/usr/include/bits/string3.h:53:10: error: ‘__builtin_memcpy’: specified size 18446744073709551615 exceeds maximum object size 9223372036854775807 [-Werror=stringop-overflow=]
   return __builtin___memcpy_chk (__dest, __src, __len, __bos0 (__dest));
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In function ‘memcpy’,
    inlined from ‘iov_to_buf’ at /mnt/kvm/qemu/cleanup/include/qemu/iov.h:64:9,
    inlined from ‘test_to_from_buf_1’ at /mnt/kvm/qemu/cleanup/tests/test-iov.c:94:14,
    inlined from ‘test_to_from_buf’ at /mnt/kvm/qemu/cleanup/tests/test-iov.c:144:9:
/usr/include/bits/string3.h:53:10: error: ‘__builtin_memcpy’: specified size 18446744073709551615 exceeds maximum object size 9223372036854775807 [-Werror=stringop-overflow=]
   return __builtin___memcpy_chk (__dest, __src, __len, __bos0 (__dest));
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors

In this case, we are abusing the functions and call a size_t argument
with -1, which gives us a very big number.  gcc gets overzealous and
gets confused about it.  Notice that this was introduced in 2015 by Paolo:

commit ad523bca56a7202d2498c550a41be5c986c4d33c
Author: Paolo Bonzini <pbonzini@redhat.com>
Date:   Tue Dec 22 12:03:33 2015 +0100

    iov: avoid memcpy for "simple" iov_from_buf/iov_to_buf

I fixed it by using the real sizes in the tests insntead of -1.  It is already calculated.




In file included from /usr/include/glib-2.0/glib/glist.h:32:0,
                 from /usr/include/glib-2.0/glib/ghash.h:33,
                 from /usr/include/glib-2.0/glib.h:50,
                 from /mnt/kvm/qemu/cleanup/tests/vmgenid-test.c:11:
In function ‘acpi_find_vgia’,
    inlined from ‘read_guid_from_memory’ at /mnt/kvm/qemu/cleanup/tests/vmgenid-test.c:103:18:
/usr/include/glib-2.0/glib/gmem.h:216:10: error: argument 1 range [18446744071562067968, 18446744073709551615] exceeds maximum object size 9223372036854775807 [-Werror=alloc-size-larger-than=]
      __p = g_##func##_n (__n, __s);   \
      ~~~~^~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/glib-2.0/glib/gmem.h:278:42: note: in expansion of macro ‘_G_NEW’
 #define g_new0(struct_type, n_structs)   _G_NEW (struct_type, n_structs, malloc0)
                                          ^~~~~~
/mnt/kvm/qemu/cleanup/tests/vmgenid-test.c:70:14: note: in expansion of macro ‘g_new0’
     tables = g_new0(uint32_t, tables_nr);
              ^~~~~~
/mnt/kvm/qemu/cleanup/tests/vmgenid-test.c: In function ‘read_guid_from_memory’:
/usr/include/glib-2.0/glib/gmem.h:96:10: note: in a call to allocation function ‘g_malloc0_n’ declared here
 gpointer g_malloc0_n      (gsize  n_blocks,
          ^~~~~~~~~~~
cc1: all warnings being treated as errors
make: *** [/mnt/kvm/qemu/cleanup/rules.mak:66: tests/vmgenid-test.o] Error 1


this cames form line:

    tables = g_new0(uint32_t, tables_nr);

glib/gcc gets completely confused about this and think that 1st
argument can be 2^64-1.  Documentation says that you should use g_new
for struct types, so ...  I moved to g_malloc0() and call it a day.


-- 
2.13.5



Juan Quintela (2):
  tests: Use real size for iov tests
  tests: Make vmgenid test compile

 include/qemu/iov.h   |  6 ------
 tests/test-iov.c     | 10 +++++-----
 tests/vmgenid-test.c |  4 ++--
 3 files changed, 7 insertions(+), 13 deletions(-)

-- 
2.13.5

[Qemu-devel] [PATCH v4 1/2] tests: Use real size for iov tests

[Juan Quintela]

We were using -1 instead of the real size because the functions check
what is bigger, size in bytes or the size of the iov.  Recent gcc's
barf at this.

Signed-off-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
Tested-by: Cleber Rosa <crosa@redhat.com>
--

Remove comments about this feature.
Fix missing -1.
---
 include/qemu/iov.h |  6 ------
 tests/test-iov.c   | 10 +++++-----
 2 files changed, 5 insertions(+), 11 deletions(-)

diff --git a/include/qemu/iov.h b/include/qemu/iov.h
index bd9fd55b0a..72d4c559b4 100644
--- a/include/qemu/iov.h
+++ b/include/qemu/iov.h
@@ -31,11 +31,6 @@ size_t iov_size(const struct iovec *iov, const unsigned int iov_cnt);
  * Number of bytes actually copied will be returned, which is
  *  min(bytes, iov_size(iov)-offset)
  * `Offset' must point to the inside of iovec.
- * It is okay to use very large value for `bytes' since we're
- * limited by the size of the iovec anyway, provided that the
- * buffer pointed to by buf has enough space.  One possible
- * such "large" value is -1 (sinice size_t is unsigned),
- * so specifying `-1' as `bytes' means 'up to the end of iovec'.
  */
 size_t iov_from_buf_full(const struct iovec *iov, unsigned int iov_cnt,
                          size_t offset, const void *buf, size_t bytes);
@@ -76,7 +71,6 @@ iov_to_buf(const struct iovec *iov, const unsigned int iov_cnt,
  * up to the end of it, will be filled with the specified value.
  * Function return actual number of bytes processed, which is
  * min(size, iov_size(iov) - offset).
- * Again, it is okay to use large value for `bytes' to mean "up to the end".
  */
 size_t iov_memset(const struct iovec *iov, const unsigned int iov_cnt,
                   size_t offset, int fillc, size_t bytes);
diff --git a/tests/test-iov.c b/tests/test-iov.c
index fa3d75aee1..458ca25099 100644
--- a/tests/test-iov.c
+++ b/tests/test-iov.c
@@ -81,17 +81,17 @@ static void test_to_from_buf_1(void)
           * skip whole vector and process exactly 0 bytes */
 
          /* first set bytes [i..sz) to some "random" value */
-         n = iov_memset(iov, niov, 0, 0xff, -1);
+         n = iov_memset(iov, niov, 0, 0xff, sz);
          g_assert(n == sz);
 
          /* next copy bytes [i..sz) from ibuf to iovec */
-         n = iov_from_buf(iov, niov, i, ibuf + i, -1);
+         n = iov_from_buf(iov, niov, i, ibuf + i, sz - i);
          g_assert(n == sz - i);
 
          /* clear part of obuf */
          memset(obuf + i, 0, sz - i);
          /* and set this part of obuf to values from iovec */
-         n = iov_to_buf(iov, niov, i, obuf + i, -1);
+         n = iov_to_buf(iov, niov, i, obuf + i, sz - i);
          g_assert(n == sz - i);
 
          /* now compare resulting buffers */
@@ -109,7 +109,7 @@ static void test_to_from_buf_1(void)
               * with j in [i..sz]. */
 
              /* clear iovec */
-             n = iov_memset(iov, niov, 0, 0xff, -1);
+             n = iov_memset(iov, niov, 0, 0xff, sz);
              g_assert(n == sz);
 
              /* copy bytes [i..j) from ibuf to iovec */
@@ -225,7 +225,7 @@ static void test_io(void)
        for (i = 0; i <= sz; ++i) {
            for (j = i; j <= sz; ++j) {
                k = i;
-               iov_memset(iov, niov, 0, 0xff, -1);
+               iov_memset(iov, niov, 0, 0xff, sz);
                do {
                    s = g_test_rand_int_range(0, j - k + 1);
                    r = iov_recv(sv[0], iov, niov, k, s);
-- 
2.13.5

Re: [Qemu-devel] [PATCH v4 1/2] tests: Use real size for iov tests

[Daniel P. Berrange]

On Fri, Sep 01, 2017 at 01:54:59PM +0200, Juan Quintela wrote:
> We were using -1 instead of the real size because the functions check
> what is bigger, size in bytes or the size of the iov.  Recent gcc's
> barf at this.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> Reviewed-by: Peter Xu <peterx@redhat.com>
> Tested-by: Cleber Rosa <crosa@redhat.com>
> --
> 
> Remove comments about this feature.
> Fix missing -1.
> ---
>  include/qemu/iov.h |  6 ------
>  tests/test-iov.c   | 10 +++++-----
>  2 files changed, 5 insertions(+), 11 deletions(-)
> 
> diff --git a/include/qemu/iov.h b/include/qemu/iov.h
> index bd9fd55b0a..72d4c559b4 100644
> --- a/include/qemu/iov.h
> +++ b/include/qemu/iov.h
> @@ -31,11 +31,6 @@ size_t iov_size(const struct iovec *iov, const unsigned int iov_cnt);
>   * Number of bytes actually copied will be returned, which is
>   *  min(bytes, iov_size(iov)-offset)
>   * `Offset' must point to the inside of iovec.
> - * It is okay to use very large value for `bytes' since we're
> - * limited by the size of the iovec anyway, provided that the
> - * buffer pointed to by buf has enough space.  One possible
> - * such "large" value is -1 (sinice size_t is unsigned),
> - * so specifying `-1' as `bytes' means 'up to the end of iovec'.
>   */
>  size_t iov_from_buf_full(const struct iovec *iov, unsigned int iov_cnt,
>                           size_t offset, const void *buf, size_t bytes);
> @@ -76,7 +71,6 @@ iov_to_buf(const struct iovec *iov, const unsigned int iov_cnt,
>   * up to the end of it, will be filled with the specified value.
>   * Function return actual number of bytes processed, which is
>   * min(size, iov_size(iov) - offset).
> - * Again, it is okay to use large value for `bytes' to mean "up to the end".
>   */
>  size_t iov_memset(const struct iovec *iov, const unsigned int iov_cnt,
>                    size_t offset, int fillc, size_t bytes);

I'm not sure its right to be removing these comments, unless you've
audited the code to ensure no caller outside the test suite is
relying on this documented behaviour.

> diff --git a/tests/test-iov.c b/tests/test-iov.c
> index fa3d75aee1..458ca25099 100644
> --- a/tests/test-iov.c
> +++ b/tests/test-iov.c
> @@ -81,17 +81,17 @@ static void test_to_from_buf_1(void)
>            * skip whole vector and process exactly 0 bytes */
>  
>           /* first set bytes [i..sz) to some "random" value */
> -         n = iov_memset(iov, niov, 0, 0xff, -1);
> +         n = iov_memset(iov, niov, 0, 0xff, sz);
>           g_assert(n == sz);
>  
>           /* next copy bytes [i..sz) from ibuf to iovec */
> -         n = iov_from_buf(iov, niov, i, ibuf + i, -1);
> +         n = iov_from_buf(iov, niov, i, ibuf + i, sz - i);
>           g_assert(n == sz - i);
>  
>           /* clear part of obuf */
>           memset(obuf + i, 0, sz - i);
>           /* and set this part of obuf to values from iovec */
> -         n = iov_to_buf(iov, niov, i, obuf + i, -1);
> +         n = iov_to_buf(iov, niov, i, obuf + i, sz - i);
>           g_assert(n == sz - i);
>  
>           /* now compare resulting buffers */
> @@ -109,7 +109,7 @@ static void test_to_from_buf_1(void)
>                * with j in [i..sz]. */
>  
>               /* clear iovec */
> -             n = iov_memset(iov, niov, 0, 0xff, -1);
> +             n = iov_memset(iov, niov, 0, 0xff, sz);
>               g_assert(n == sz);
>  
>               /* copy bytes [i..j) from ibuf to iovec */
> @@ -225,7 +225,7 @@ static void test_io(void)
>         for (i = 0; i <= sz; ++i) {
>             for (j = i; j <= sz; ++j) {
>                 k = i;
> -               iov_memset(iov, niov, 0, 0xff, -1);
> +               iov_memset(iov, niov, 0, 0xff, sz);
>                 do {
>                     s = g_test_rand_int_range(0, j - k + 1);
>                     r = iov_recv(sv[0], iov, niov, k, s);

For the test-iov.c changes though you can have

Reviewed-by: Daniel P. Berrange <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v4 1/2] tests: Use real size for iov tests

[Juan Quintela]

"Daniel P. Berrange" <berrange@redhat.com> wrote:
> On Fri, Sep 01, 2017 at 01:54:59PM +0200, Juan Quintela wrote:
>> We were using -1 instead of the real size because the functions check
>> what is bigger, size in bytes or the size of the iov.  Recent gcc's
>> barf at this.
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> Reviewed-by: Peter Xu <peterx@redhat.com>
>> Tested-by: Cleber Rosa <crosa@redhat.com>
>> --
>> 
>> Remove comments about this feature.
>> Fix missing -1.
>> ---
>>  include/qemu/iov.h |  6 ------
>>  tests/test-iov.c   | 10 +++++-----
>>  2 files changed, 5 insertions(+), 11 deletions(-)
>> 
>> diff --git a/include/qemu/iov.h b/include/qemu/iov.h
>> index bd9fd55b0a..72d4c559b4 100644
>> --- a/include/qemu/iov.h
>> +++ b/include/qemu/iov.h
>> @@ -31,11 +31,6 @@ size_t iov_size(const struct iovec *iov, const unsigned int iov_cnt);
>>   * Number of bytes actually copied will be returned, which is
>>   *  min(bytes, iov_size(iov)-offset)
>>   * `Offset' must point to the inside of iovec.
>> - * It is okay to use very large value for `bytes' since we're
>> - * limited by the size of the iovec anyway, provided that the
>> - * buffer pointed to by buf has enough space.  One possible
>> - * such "large" value is -1 (sinice size_t is unsigned),
>> - * so specifying `-1' as `bytes' means 'up to the end of iovec'.
>>   */
>>  size_t iov_from_buf_full(const struct iovec *iov, unsigned int iov_cnt,
>>                           size_t offset, const void *buf, size_t bytes);
>> @@ -76,7 +71,6 @@ iov_to_buf(const struct iovec *iov, const unsigned int iov_cnt,
>>   * up to the end of it, will be filled with the specified value.
>>   * Function return actual number of bytes processed, which is
>>   * min(size, iov_size(iov) - offset).
>> - * Again, it is okay to use large value for `bytes' to mean "up to the end".
>>   */
>>  size_t iov_memset(const struct iovec *iov, const unsigned int iov_cnt,
>>                    size_t offset, int fillc, size_t bytes);
>
> I'm not sure its right to be removing these comments, unless you've
> audited the code to ensure no caller outside the test suite is
> relying on this documented behaviour.

I did.  The caller call with the real size of the buffer.
iov_memset() still works with the -1 size.

iov_to/from_buf, the non full versions don't work, because the inline
gets confused with -1 with newer gcc's.
>
> For the test-iov.c changes though you can have
>
> Reviewed-by: Daniel P. Berrange <berrange@redhat.com>

Later, Juan.

[Qemu-devel] [PATCH v4 2/2] tests: Make vmgenid test compile

[Juan Quintela]

Just make sure that nr_tables is size_t not int.
Once there, do the assert in the right place and be sure that we don't
have a division by zero.

Suggested-by: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Juan Quintela <quintela@redhat.com>
Tested-by: Cleber Rosa <crosa@redhat.com>

--

Drop the s/g_new0/g_malloc0/ change.
Avoid division by zero with assert (danp)
---
 tests/vmgenid-test.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/vmgenid-test.c b/tests/vmgenid-test.c
index 3d5c1c3615..918c82c82a 100644
--- a/tests/vmgenid-test.c
+++ b/tests/vmgenid-test.c
@@ -40,7 +40,7 @@ static uint32_t acpi_find_vgia(void)
     AcpiRsdpDescriptor rsdp_table;
     uint32_t rsdt;
     AcpiRsdtDescriptorRev1 rsdt_table;
-    int tables_nr;
+    size_t tables_nr;
     uint32_t *tables;
     AcpiTableHeader ssdt_table;
     VgidTable vgid_table;
@@ -62,9 +62,9 @@ static uint32_t acpi_find_vgia(void)
     ACPI_ASSERT_CMP(rsdt_table.signature, "RSDT");
 
     /* compute the table entries in rsdt */
+    g_assert_cmpint(rsdt_table.length, >, sizeof(AcpiRsdtDescriptorRev1));
     tables_nr = (rsdt_table.length - sizeof(AcpiRsdtDescriptorRev1)) /
                 sizeof(uint32_t);
-    g_assert_cmpint(tables_nr, >, 0);
 
     /* get the addresses of the tables pointed by rsdt */
     tables = g_new0(uint32_t, tables_nr);
-- 
2.13.5

Re: [Qemu-devel] [PATCH v4 2/2] tests: Make vmgenid test compile

[Daniel P. Berrange]

On Fri, Sep 01, 2017 at 01:55:00PM +0200, Juan Quintela wrote:
> Just make sure that nr_tables is size_t not int.
> Once there, do the assert in the right place and be sure that we don't
> have a division by zero.
> 
> Suggested-by: Cédric Le Goater <clg@kaod.org>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> Tested-by: Cleber Rosa <crosa@redhat.com>

Reviewed-by: Daniel P. Berrange <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|