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

["Daniel P. Berrange" <berrange@redhat.com>]

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 :|