Message ID | 20170720163536.26183-1-dgilbert@redhat.com |
---|---|
State | New |
Headers | show |
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
* 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
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 >
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);