Re: [Qemu-devel] [PATCH] tests/bios-tables-test: Compiler warning fix

[Marcel Apfelbaum]

On 20/07/2017 19:55, Dr. David Alan Gilbert wrote:
> * Daniel P. Berrange (address@hidden) wrote:
> > On Thu, Jul 20, 2017 at 05:35:36PM +0100, Dr. David Alan Gilbert (git) wrote:
> > > From: "Dr. David Alan Gilbert" <address@hidden>
> > >
> > > gcc 7.1.1 in fedora 26 moans about the:
> > >     tables = g_new0(uint32_t, tables_nr)
> > >
> > > because it can't convince itself that tables_nr is positive.
> > > This is fallout from g_assert_cmpint no longer necessarily being
> > > no-return;  replace it with a plain g_assert.
> > >
> > > Signed-off-by: Dr. David Alan Gilbert <address@hidden>
> > > ---
> > >   tests/bios-tables-test.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> > > index 63da978f0b..564da45f65 100644
> > > --- a/tests/bios-tables-test.c
> > > +++ b/tests/bios-tables-test.c
> > > @@ -108,7 +108,7 @@ static void test_acpi_rsdt_table(test_data *data)
> > >       /* compute the table entries in rsdt */
> > >       tables_nr = (rsdt_table->length - sizeof(AcpiRsdtDescriptorRev1)) /
> > >                   sizeof(uint32_t);
> > > -    g_assert_cmpint(tables_nr, >, 0);
> > > +    g_assert(tables_nr > 0);
> >
> > IMHO your original patch was better - rsdt_table->length is an
> > uint32_t, and sizeof() evaluates to size_t, but we're assigning
> > to a local  tables_nr that is a signed int. So tables_nr would
> > be better declared as size_t.  That would mean the assert can
> > just be removed entirely, or replaced by an assert that
> > rsdt_table->length > sizeof(AcpiRsdtDescriptorRev1) if we're
> > concerned about that
>
> Given that assert was there I'd assumed Marcel was explicitly checking
> for that case, so my original hack of just making it a uint or size_t
> didn't seem safe.
>
> Checking that rsdt_table->length > sizeof(AcpiRsdtDescriptorRev1)
> probably is safer; although this is only a sanity assert in a test
> case, not dealing with user data, so I just made the minimal change.


I personally think is enough, since we only
want to keep gcc happy.

Reviewed-by: Marcel Apfelbaum <address@hidden>

Thanks,
Marcel

> Dave
>
> > 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 :|
> --
> Dr. David Alan Gilbert / address@hidden / Manchester, UK