tests/bios-tables-test: Compiler warning fix

PATCH In review Dr. David Alan Gilbert (git) 3 participants 4 messages Jul 20, 2017 – Jul 21, 2017

lore

Share & tools

lore

Dr. David Alan Gilbert (git)

author

2017-07-20 16:35 UTC #0 lore raw

PATCH 1/1 tests/bios-tables-test: Compiler warning fix

From: "Dr. David Alan Gilbert" <[email protected]>

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 <[email protected]>

---
 tests/bios-tables-test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

tests/bios-tables-test.c +1 -1

index 63da978f0b..564da45f65 100644
@@ -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);

Signature · 1 line

2.13.3

Daniel P. Berrange

2017-07-20 16:44 UTC #1 lore raw

Quoted 19 lines · Dr. David Alan Gilbert (git)

From: "Dr. David Alan Gilbert" <[email protected]>
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 <[email protected]>
---
 tests/bios-tables-test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
tests/bios-tables-test.c +0 -0
index 63da978f0b..564da45f65 100644

tests/bios-tables-test.c static void test_acpi_rsdt_table(test_data *data) · line 111

Quoted 5 lines

tests/bios-tables-test.c +1 -1

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

Signature · 3 lines

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

author

2017-07-20 16:56 UTC #2 lore raw

Quoted 26 lines · * Daniel P. Berrange ([email protected]) wrote:

Quoted 19 lines · Dr. David Alan Gilbert (git)
From: "Dr. David Alan Gilbert" <[email protected]>
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 <[email protected]>
---
 tests/bios-tables-test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
tests/bios-tables-test.c +0 -0
index 63da978f0b..564da45f65 100644
tests/bios-tables-test.c static void test_acpi_rsdt_table(test_data *data) · line 111
Quoted 5 lines
tests/bios-tables-test.c +1 -1
@@ -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);

Quoted 7 lines

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

Quoted 7 lines

Regards,
Daniel

Signature · 3 lines

|: 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 / [email protected] / Manchester, UK

Marcel Apfelbaum

Reviewed-by

2017-07-21 14:48 UTC #3 lore raw

Quoted 35 lines · Dr. David Alan Gilbert

Quoted 26 lines · * Daniel P. Berrange ([email protected]) wrote:
Quoted 19 lines · Dr. David Alan Gilbert (git)
From: "Dr. David Alan Gilbert" <[email protected]>
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 <[email protected]>
---
  tests/bios-tables-test.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
tests/bios-tables-test.c +0 -0
index 63da978f0b..564da45f65 100644
tests/bios-tables-test.c static void test_acpi_rsdt_table(test_data *data) · line 111
Quoted 5 lines
tests/bios-tables-test.c +1 -1
@@ -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);
Quoted 7 lines
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

Quoted 8 lines

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 <[email protected]>

Thanks,
Marcel

Quoted 12 lines

Dave
Quoted 7 lines
Regards,
Daniel
Signature · 3 lines
|: 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 / [email protected] / Manchester, UK