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

[Juan Quintela <quintela@redhat.com>]

"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.