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

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

[Juan Quintela]

Hi

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.

What do you think?

Later, Juan.






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

 tests/test-iov.c     | 8 ++++----
 tests/vmgenid-test.c | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

-- 
2.13.5

[Qemu-devel] [PATCH 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>
---
 tests/test-iov.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tests/test-iov.c b/tests/test-iov.c
index a22d71fd2c..819c410a51 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 */
-- 
2.13.5

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

[Juan Quintela]

Compiler gets confused with the size of the struct, so move form
g_new0() to g_malloc0().

I *think* that the problem is in gcc (or glib for that matter), but
the documentation of the g_new0 states that 1sts first argument is an
struct type, and uint32_t is not an struct type.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 tests/vmgenid-test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/vmgenid-test.c b/tests/vmgenid-test.c
index 3d5c1c3615..032e1d465a 100644
--- a/tests/vmgenid-test.c
+++ b/tests/vmgenid-test.c
@@ -67,7 +67,7 @@ static uint32_t acpi_find_vgia(void)
     g_assert_cmpint(tables_nr, >, 0);
 
     /* get the addresses of the tables pointed by rsdt */
-    tables = g_new0(uint32_t, tables_nr);
+    tables = g_malloc0(sizeof(uint32_t) * tables_nr);
     ACPI_READ_ARRAY_PTR(tables, tables_nr, rsdt);
 
     for (i = 0; i < tables_nr; i++) {
-- 
2.13.5

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

[Peter Xu]

On Wed, Aug 23, 2017 at 10:39:00AM +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>
> ---
>  tests/test-iov.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/test-iov.c b/tests/test-iov.c
> index a22d71fd2c..819c410a51 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);

This one is not needed?

>           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);

This one as well?

Actually I think we can keep the two places above, but there seems to
be a 3rd one below which is untouched.  If we do change the two, maybe
we'd better change the 3rd one as well.

Besides:

Reviewed-by: Peter Xu <peterx@redhat.com>

>               g_assert(n == sz);
>  
>               /* copy bytes [i..j) from ibuf to iovec */
> -- 
> 2.13.5
> 

-- 
Peter Xu

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

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> On Wed, Aug 23, 2017 at 10:39:00AM +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>
>> ---
>>  tests/test-iov.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>> 
>> diff --git a/tests/test-iov.c b/tests/test-iov.c
>> index a22d71fd2c..819c410a51 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);
>
> This one is not needed?

Not, but it is for consistence.  iov_memset() has that property, that it
memset whatever is smaller, bytes parameter or iov size.


>>                * with j in [i..sz]. */
>>  
>>               /* clear iovec */
>> -             n = iov_memset(iov, niov, 0, 0xff, -1);
>> +             n = iov_memset(iov, niov, 0, 0xff, sz);
>
> This one as well?

I decided to change all of them for consistence.

Using -1 is a trick that just happens to work for current
implementation, but it is a hack.  and we have just after that an assert
with the real size that we want to copy.  Just use them everywhere.

>
> Actually I think we can keep the two places above, but there seems to
> be a 3rd one below which is untouched.  If we do change the two, maybe
> we'd better change the 3rd one as well.

Opps, didn't saw that other.  With this changes it fixed all for me.



> Besides:
>
> Reviewed-by: Peter Xu <peterx@redhat.com>

Thanks, Juan.


>
>>               g_assert(n == sz);
>>  
>>               /* copy bytes [i..j) from ibuf to iovec */
>> -- 
>> 2.13.5
>> 

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

[Dr. David Alan Gilbert]

* Juan Quintela (quintela@redhat.com) wrote:
> Compiler gets confused with the size of the struct, so move form
> g_new0() to g_malloc0().
> 
> I *think* that the problem is in gcc (or glib for that matter), but
> the documentation of the g_new0 states that 1sts first argument is an
> struct type, and uint32_t is not an struct type.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  tests/vmgenid-test.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/vmgenid-test.c b/tests/vmgenid-test.c
> index 3d5c1c3615..032e1d465a 100644
> --- a/tests/vmgenid-test.c
> +++ b/tests/vmgenid-test.c
> @@ -67,7 +67,7 @@ static uint32_t acpi_find_vgia(void)
>      g_assert_cmpint(tables_nr, >, 0);
>  
>      /* get the addresses of the tables pointed by rsdt */
> -    tables = g_new0(uint32_t, tables_nr);
> +    tables = g_malloc0(sizeof(uint32_t) * tables_nr);

I think there's an easier fix for this I think;
try:

-    g_assert_cmpint(tables_nr, >, 0);
+    g_assert(tables_nr > 0);

Dave

>      ACPI_READ_ARRAY_PTR(tables, tables_nr, rsdt);
>  
>      for (i = 0; i < tables_nr; i++) {
> -- 
> 2.13.5
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Cédric Le Goater]

On 08/23/2017 01:53 PM, Dr. David Alan Gilbert wrote:
> * Juan Quintela (quintela@redhat.com) wrote:
>> Compiler gets confused with the size of the struct, so move form
>> g_new0() to g_malloc0().
>>
>> I *think* that the problem is in gcc (or glib for that matter), but
>> the documentation of the g_new0 states that 1sts first argument is an
>> struct type, and uint32_t is not an struct type.
>>
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  tests/vmgenid-test.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tests/vmgenid-test.c b/tests/vmgenid-test.c
>> index 3d5c1c3615..032e1d465a 100644
>> --- a/tests/vmgenid-test.c
>> +++ b/tests/vmgenid-test.c
>> @@ -67,7 +67,7 @@ static uint32_t acpi_find_vgia(void)
>>      g_assert_cmpint(tables_nr, >, 0);
>>  
>>      /* get the addresses of the tables pointed by rsdt */
>> -    tables = g_new0(uint32_t, tables_nr);
>> +    tables = g_malloc0(sizeof(uint32_t) * tables_nr);
> 
> I think there's an easier fix for this I think;
> try:
> 
> -    g_assert_cmpint(tables_nr, >, 0);
> +    g_assert(tables_nr > 0);

I fixed that one with :

@@ -40,7 +40,7 @@ static uint32_t acpi_find_vgia(void)
     AcpiRsdpDescriptor rsdp_table;
     uint32_t rsdt;
     AcpiRsdtDescriptorRev1 rsdt_table;
-    int tables_nr;
+    uint32_t tables_nr;
     uint32_t *tables;
     AcpiTableHeader ssdt_table;
     VgidTable vgid_table;


C. 



> Dave
> 
>>      ACPI_READ_ARRAY_PTR(tables, tables_nr, rsdt);
>>  
>>      for (i = 0; i < tables_nr; i++) {
>> -- 
>> 2.13.5
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 

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

[Thomas Huth]

On 23.08.2017 10:39, 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>
> ---
>  tests/test-iov.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

While you're at it, could you maybe also adjust the comments in
include/qemu/iov.h ? It currently says:

 * 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'.

... and apparently -1 is not working anymore as expected. Maybe SIZE_MAX
from stdint.h is a better choice?

 Thomas

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

[Eric Blake]

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

On 08/28/2017 09:41 AM, Cédric Le Goater wrote:
> On 08/23/2017 01:53 PM, Dr. David Alan Gilbert wrote:
>> * Juan Quintela (quintela@redhat.com) wrote:
>>> Compiler gets confused with the size of the struct, so move form
>>> g_new0() to g_malloc0().
>>>
>>> I *think* that the problem is in gcc (or glib for that matter), but
>>> the documentation of the g_new0 states that 1sts first argument is an
>>> struct type, and uint32_t is not an struct type.
>>>
>>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>>> ---

>>>  
>>>      /* get the addresses of the tables pointed by rsdt */
>>> -    tables = g_new0(uint32_t, tables_nr);
>>> +    tables = g_malloc0(sizeof(uint32_t) * tables_nr);
>>

> I fixed that one with :
> 
> @@ -40,7 +40,7 @@ static uint32_t acpi_find_vgia(void)
>      AcpiRsdpDescriptor rsdp_table;
>      uint32_t rsdt;
>      AcpiRsdtDescriptorRev1 rsdt_table;
> -    int tables_nr;
> +    uint32_t tables_nr;

I like this one better (multiplication in g_malloc0() makes me worry
about overflow; using unsigned math to avoid the problem is nicer).  Are
we going to see a v2 of this patch series?

-- 
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 1/2] tests: Use real size for iov tests

[Juan Quintela]

Thomas Huth <thuth@redhat.com> wrote:
> On 23.08.2017 10:39, 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>
>> ---
>>  tests/test-iov.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> While you're at it, could you maybe also adjust the comments in
> include/qemu/iov.h ? It currently says:
>
>  * 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'.

Haven't found this.  Fixing.

>
> ... and apparently -1 is not working anymore as expected. Maybe SIZE_MAX
> from stdint.h is a better choice?

Compiler get confused with it, because we have _new_ values, see the
warning on the cover, I *think* that in this one, the compiler is *kind*
of right. (*)

>  Thomas

Later, Juan.

(*) see the warning, I think that it gets a bit confused, but generaly,
    we are just abusing the interface, and anyways, we are using in real
    code the real size, and in the test, it was easy to change
    everything to the real size, so I think it is better just to remove
    *that* feature.

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

[Daniel P. Berrange]

On Tue, Aug 29, 2017 at 03:17:25PM -0500, Eric Blake wrote:
> On 08/28/2017 09:41 AM, Cédric Le Goater wrote:
> > On 08/23/2017 01:53 PM, Dr. David Alan Gilbert wrote:
> >> * Juan Quintela (quintela@redhat.com) wrote:
> >>> Compiler gets confused with the size of the struct, so move form
> >>> g_new0() to g_malloc0().
> >>>
> >>> I *think* that the problem is in gcc (or glib for that matter), but
> >>> the documentation of the g_new0 states that 1sts first argument is an
> >>> struct type, and uint32_t is not an struct type.
> >>>
> >>> Signed-off-by: Juan Quintela <quintela@redhat.com>
> >>> ---
> 
> >>>  
> >>>      /* get the addresses of the tables pointed by rsdt */
> >>> -    tables = g_new0(uint32_t, tables_nr);
> >>> +    tables = g_malloc0(sizeof(uint32_t) * tables_nr);
> >>
> 
> > I fixed that one with :
> > 
> > @@ -40,7 +40,7 @@ static uint32_t acpi_find_vgia(void)
> >      AcpiRsdpDescriptor rsdp_table;
> >      uint32_t rsdt;
> >      AcpiRsdtDescriptorRev1 rsdt_table;
> > -    int tables_nr;
> > +    uint32_t tables_nr;
> 
> I like this one better (multiplication in g_malloc0() makes me worry
> about overflow; using unsigned math to avoid the problem is nicer).  Are
> we going to see a v2 of this patch series?

It should really be size_t, because it is assigned from the result of
a size_t calculation, but you then also need to change a later assert
which was relying on it being signed:

@@ -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);


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 2/2] tests: Make acpid test compile

[Juan Quintela]

Cedric Le Goater <clg@kaod.org> wrote:
> On 08/23/2017 01:53 PM, Dr. David Alan Gilbert wrote:
>> * Juan Quintela (quintela@redhat.com) wrote:
>>> Compiler gets confused with the size of the struct, so move form
>>> g_new0() to g_malloc0().
>>>
>>> I *think* that the problem is in gcc (or glib for that matter), but
>>> the documentation of the g_new0 states that 1sts first argument is an
>>> struct type, and uint32_t is not an struct type.
>>>
>>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>>> ---
>>>  tests/vmgenid-test.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/tests/vmgenid-test.c b/tests/vmgenid-test.c
>>> index 3d5c1c3615..032e1d465a 100644
>>> --- a/tests/vmgenid-test.c
>>> +++ b/tests/vmgenid-test.c
>>> @@ -67,7 +67,7 @@ static uint32_t acpi_find_vgia(void)
>>>      g_assert_cmpint(tables_nr, >, 0);
>>>  
>>>      /* get the addresses of the tables pointed by rsdt */
>>> -    tables = g_new0(uint32_t, tables_nr);
>>> +    tables = g_malloc0(sizeof(uint32_t) * tables_nr);
>> 
>> I think there's an easier fix for this I think;
>> try:
>> 
>> -    g_assert_cmpint(tables_nr, >, 0);
>> +    g_assert(tables_nr > 0);

I liked more the following one.


>
> I fixed that one with :
>
> @@ -40,7 +40,7 @@ static uint32_t acpi_find_vgia(void)
>      AcpiRsdpDescriptor rsdp_table;
>      uint32_t rsdt;
>      AcpiRsdtDescriptorRev1 rsdt_table;
> -    int tables_nr;
> +    uint32_t tables_nr;
>      uint32_t *tables;
>      AcpiTableHeader ssdt_table;
>      VgidTable vgid_table;


This make things work for me, so moving to this one.

Thanks, Juan.

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

[Daniel P. Berrange]

On Wed, Aug 30, 2017 at 12:51:23PM +0200, Juan Quintela wrote:
> Cedric Le Goater <clg@kaod.org> wrote:
> > On 08/23/2017 01:53 PM, Dr. David Alan Gilbert wrote:
> >> * Juan Quintela (quintela@redhat.com) wrote:
> >>> Compiler gets confused with the size of the struct, so move form
> >>> g_new0() to g_malloc0().
> >>>
> >>> I *think* that the problem is in gcc (or glib for that matter), but
> >>> the documentation of the g_new0 states that 1sts first argument is an
> >>> struct type, and uint32_t is not an struct type.
> >>>
> >>> Signed-off-by: Juan Quintela <quintela@redhat.com>
> >>> ---
> >>>  tests/vmgenid-test.c | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/tests/vmgenid-test.c b/tests/vmgenid-test.c
> >>> index 3d5c1c3615..032e1d465a 100644
> >>> --- a/tests/vmgenid-test.c
> >>> +++ b/tests/vmgenid-test.c
> >>> @@ -67,7 +67,7 @@ static uint32_t acpi_find_vgia(void)
> >>>      g_assert_cmpint(tables_nr, >, 0);
> >>>  
> >>>      /* get the addresses of the tables pointed by rsdt */
> >>> -    tables = g_new0(uint32_t, tables_nr);
> >>> +    tables = g_malloc0(sizeof(uint32_t) * tables_nr);
> >> 
> >> I think there's an easier fix for this I think;
> >> try:
> >> 
> >> -    g_assert_cmpint(tables_nr, >, 0);
> >> +    g_assert(tables_nr > 0);
> 
> I liked more the following one.
> 
> 
> >
> > I fixed that one with :
> >
> > @@ -40,7 +40,7 @@ static uint32_t acpi_find_vgia(void)
> >      AcpiRsdpDescriptor rsdp_table;
> >      uint32_t rsdt;
> >      AcpiRsdtDescriptorRev1 rsdt_table;
> > -    int tables_nr;
> > +    uint32_t tables_nr;
> >      uint32_t *tables;
> >      AcpiTableHeader ssdt_table;
> >      VgidTable vgid_table;
> 
> 
> This make things work for me, so moving to this one.

It should be size_t, because its taking the result of a calculation that
is size_t.  You also need to change the assert I mention in my other email.


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 1/2] tests: Use real size for iov tests

[Juan Quintela]

Thomas Huth <thuth@redhat.com> wrote:
> On 23.08.2017 10:39, 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>
>> ---
>>  tests/test-iov.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> While you're at it, could you maybe also adjust the comments in
> include/qemu/iov.h ? It currently says:
>
>  * 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'.

This is for the _full() versions, and still work.  the same for
iov_memset().  

> ... and apparently -1 is not working anymore as expected. Maybe SIZE_MAX
> from stdint.h is a better choice?


static inline size_t
iov_from_buf(const struct iovec *iov, unsigned int iov_cnt,
             size_t offset, const void *buf, size_t bytes)
{
    if (__builtin_constant_p(bytes) && iov_cnt &&
        offset <= iov[0].iov_len && bytes <= iov[0].iov_len - offset) {
        memcpy(iov[0].iov_base + offset, buf, bytes);
        return bytes;
    } else {
        return iov_from_buf_full(iov, iov_cnt, offset, buf, bytes);
    }
}

This optimization don't work very well if we used bytes = -1,
furthermore, we return the wrong value.

And no, I don't understand how the (bytes <= iov[0].iov_len - offset)
test pass when bytes == -1.

Later, Juan.

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

[Juan Quintela]

"Daniel P. Berrange" <berrange@redhat.com> wrote:
> On Tue, Aug 29, 2017 at 03:17:25PM -0500, Eric Blake wrote:
>> On 08/28/2017 09:41 AM, Cédric Le Goater wrote:
>> > On 08/23/2017 01:53 PM, Dr. David Alan Gilbert wrote:
>> >> * Juan Quintela (quintela@redhat.com) wrote:
>> >>> Compiler gets confused with the size of the struct, so move form
>> >>> g_new0() to g_malloc0().
>> >>>
>> >>> I *think* that the problem is in gcc (or glib for that matter), but
>> >>> the documentation of the g_new0 states that 1sts first argument is an
>> >>> struct type, and uint32_t is not an struct type.
>> >>>
>> >>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> >>> ---
>> 
>> >>>  
>> >>>      /* get the addresses of the tables pointed by rsdt */
>> >>> -    tables = g_new0(uint32_t, tables_nr);
>> >>> +    tables = g_malloc0(sizeof(uint32_t) * tables_nr);
>> >>
>> 
>> > I fixed that one with :
>> > 
>> > @@ -40,7 +40,7 @@ static uint32_t acpi_find_vgia(void)
>> >      AcpiRsdpDescriptor rsdp_table;
>> >      uint32_t rsdt;
>> >      AcpiRsdtDescriptorRev1 rsdt_table;
>> > -    int tables_nr;
>> > +    uint32_t tables_nr;
>> 
>> I like this one better (multiplication in g_malloc0() makes me worry
>> about overflow; using unsigned math to avoid the problem is nicer).  Are
>> we going to see a v2 of this patch series?
>
> It should really be size_t, because it is assigned from the result of
> a size_t calculation, but you then also need to change a later assert
> which was relying on it being signed:
>
> @@ -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;

I was using this already.

>      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);
>

And here we are, this mail arrived after I sent my new series.  Will
wait for more comments and resend later.

Thanks, Juan.