tests/bios-tables-test: Compiler warning fix

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

Posted by Dr. David Alan Gilbert (git) 6 years, 9 months ago

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

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 <dgilbert@redhat.com>
---
 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);
 
     /* get the addresses of the tables pointed by rsdt */
     tables = g_new0(uint32_t, tables_nr);
-- 
2.13.3

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

Posted by Daniel P. Berrange 6 years, 9 months ago

On Thu, Jul 20, 2017 at 05:35:36PM +0100, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> 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 <dgilbert@redhat.com>
> ---
>  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


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] tests/bios-tables-test: Compiler warning fix

Posted by Dr. David Alan Gilbert 6 years, 9 months ago

* Daniel P. Berrange (berrange@redhat.com) wrote:
> On Thu, Jul 20, 2017 at 05:35:36PM +0100, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > 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 <dgilbert@redhat.com>
> > ---
> >  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.

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 / dgilbert@redhat.com / Manchester, UK

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

Posted by Marcel Apfelbaum 6 years, 9 months ago

On 20/07/2017 19:55, Dr. David Alan Gilbert wrote:
> * Daniel P. Berrange (berrange@redhat.com) wrote:
>> On Thu, Jul 20, 2017 at 05:35:36PM +0100, Dr. David Alan Gilbert (git) wrote:
>>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>>>
>>> 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 <dgilbert@redhat.com>
>>> ---
>>>   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 <marcel@redhat.com>

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 / dgilbert@redhat.com / Manchester, UK
>