Nico Huber has submitted this change. ( https://review.coreboot.org/c/flashrom/+/51243 )
Change subject: Enable dynamic memory allocation checks for cmocka unit tests
......................................................................
Enable dynamic memory allocation checks for cmocka unit tests
This commit enables the feature and makes changes to existing
files and tests. I am writing more new tests with this.
Commit includes tests/flashrom.c because after enabling memory
checks the test started to fail (it used to leak memory indeed).
If you are wondering how to verify it works (because at the moment
all tests [still] pass so it’s not obvious that anything has
changed), then for example:
1) Remove free’s in flashbuses_to_text_test_success test, and it
will fail with message similar to this (line numbers from your local
source)
[ ERROR ] --- Blocks allocated...
../flashrom.c:1239: note: block 0x55f42304b640 allocated here
../flashrom.c:1239: note: block 0x55f42304b5c0 allocated here
../flashrom.c:1239: note: block 0x55f42304b3d0 allocated here
../flashrom.c:1239: note: block 0x55f42304b700 allocated here
../flashrom.c:1239: note: block 0x55f42304b780 allocated here
../flashrom.c:1239: note: block 0x55f42304bb00 allocated here
../flashrom.c:1239: note: block 0x55f42304b810 allocated here
ERROR: flashbuses_to_text_test_success leaked 7 block(s)
2) Add char *temp = malloc just before return from strcat_realloc
[ ERROR ] --- Blocks allocated...
../helpers.c:88: note: block 0x55a51307b6c0 allocated here
../helpers.c:88: note: block 0x55a51307b9e0 allocated here
ERROR: strcat_realloc_test_success leaked 2 block(s)
BUG=b:181803212
TEST=builds and ninja test
nm builddir/tests/flashrom_unit_tests.p/.._flashrom.c.o
nm builddir/tests/flashrom_unit_tests.p/flashrom.c.o
nm builddir/flashrom.p/flashrom.c.o
Change-Id: I0c6b6b8dc17aaee28640e3fca3d1fc9f7feabf5f
Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org>
Reviewed-on: https://review.coreboot.org/c/flashrom/+/51243
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
Reviewed-by: Nico Huber <nico.h(a)gmx.de>
Reviewed-by: Angel Pons <th3fanbus(a)gmail.com>
---
M meson.build
M tests/flashrom.c
M tests/helpers.c
A unittest_env.h
4 files changed, 81 insertions(+), 12 deletions(-)
Approvals:
build bot (Jenkins): Verified
Nico Huber: Looks good to me, approved
Angel Pons: Looks good to me, approved
diff --git a/meson.build b/meson.build
index d5895d8..5b339e0 100644
--- a/meson.build
+++ b/meson.build
@@ -472,6 +472,10 @@
'cli_output.c',
'flashrom.c',
],
+ compile_args : [
+ '-includestdlib.h',
+ '-includeunittest_env.h',
+ ],
dependencies : [
deps,
],
diff --git a/tests/flashrom.c b/tests/flashrom.c
index 50464dd..c3508c5 100644
--- a/tests/flashrom.c
+++ b/tests/flashrom.c
@@ -17,35 +17,54 @@
#include "programmer.h"
+#define assert_equal_and_free(text, expected) \
+ do { \
+ assert_string_equal(text, expected); \
+ free(text); \
+ } while (0)
+
+#define assert_not_equal_and_free(text, expected) \
+ do { \
+ assert_string_not_equal(text, expected); \
+ free(text); \
+ } while (0)
+
+
void flashbuses_to_text_test_success(void **state)
{
(void) state; /* unused */
enum chipbustype bustype;
+ char *text;
bustype = BUS_NONSPI;
- assert_string_equal(flashbuses_to_text(bustype), "Non-SPI");
+ text = flashbuses_to_text(bustype);
+ assert_equal_and_free(text, "Non-SPI");
bustype |= BUS_PARALLEL;
- assert_string_not_equal(flashbuses_to_text(bustype), "Non-SPI, Parallel");
+ text = flashbuses_to_text(bustype);
+ assert_not_equal_and_free(text, "Non-SPI, Parallel");
bustype = BUS_PARALLEL;
bustype |= BUS_LPC;
- assert_string_equal(flashbuses_to_text(bustype), "Parallel, LPC");
+ text = flashbuses_to_text(bustype);
+ assert_equal_and_free(text, "Parallel, LPC");
bustype |= BUS_FWH;
//BUS_NONSPI = BUS_PARALLEL | BUS_LPC | BUS_FWH,
- assert_string_equal(flashbuses_to_text(bustype), "Non-SPI");
+ text = flashbuses_to_text(bustype);
+ assert_equal_and_free(text, "Non-SPI");
bustype |= BUS_SPI;
- assert_string_equal(flashbuses_to_text(bustype), "Parallel, LPC, FWH, SPI");
+ text = flashbuses_to_text(bustype);
+ assert_equal_and_free(text, "Parallel, LPC, FWH, SPI");
bustype |= BUS_PROG;
- assert_string_equal(
- flashbuses_to_text(bustype),
- "Parallel, LPC, FWH, SPI, Programmer-specific"
- );
+ text = flashbuses_to_text(bustype);
+ assert_equal_and_free(text,
+ "Parallel, LPC, FWH, SPI, Programmer-specific");
bustype = BUS_NONE;
- assert_string_equal(flashbuses_to_text(bustype), "None");
+ text = flashbuses_to_text(bustype);
+ assert_equal_and_free(text, "None");
}
diff --git a/tests/helpers.c b/tests/helpers.c
index a920c15..4376eee 100644
--- a/tests/helpers.c
+++ b/tests/helpers.c
@@ -18,8 +18,6 @@
#include "flash.h"
#include <stdint.h>
-#include <stdlib.h>
-
void address_to_bits_test_success(void **state)
{
diff --git a/unittest_env.h b/unittest_env.h
new file mode 100644
index 0000000..9bd7509
--- /dev/null
+++ b/unittest_env.h
@@ -0,0 +1,48 @@
+/*
+ * This file is part of the flashrom project.
+ *
+ * Copyright 2021 Google LLC
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+/*
+ * This header is included in all files when they are built for unit test
+ * environment, for all the files to be covered by dynamic memory allocation
+ * checks (checks for memory leaks, buffer overflows and underflows).
+ *
+ * See flashrom_test_dep in meson.build for more details.
+ *
+ * https://api.cmocka.org/group__cmocka__alloc.html
+ */
+
+extern void* _test_malloc(const size_t size, const char* file, const int line);
+extern void* _test_realloc(void *ptr, const size_t size, const char* file, const int line);
+extern void* _test_calloc(const size_t number_of_elements, const size_t size,
+ const char* file, const int line);
+extern void _test_free(void* const ptr, const char* file, const int line);
+
+#ifdef malloc
+#undef malloc
+#endif
+#ifdef calloc
+#undef calloc
+#endif
+#ifdef realloc
+#undef realloc
+#endif
+#ifdef free
+#undef free
+#endif
+
+#define malloc(size) _test_malloc(size, __FILE__, __LINE__)
+#define realloc(ptr, size) _test_realloc(ptr, size, __FILE__, __LINE__)
+#define calloc(num, size) _test_calloc(num, size, __FILE__, __LINE__)
+#define free(ptr) _test_free(ptr, __FILE__, __LINE__)
--
To view, visit https://review.coreboot.org/c/flashrom/+/51243
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I0c6b6b8dc17aaee28640e3fca3d1fc9f7feabf5f
Gerrit-Change-Number: 51243
Gerrit-PatchSet: 8
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Daniel Kurtz <djkurtz(a)google.com>
Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Daniel Kurtz <djkurtz(a)chromium.org>
Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-CC: Thomas Heijligen <src(a)posteo.de>
Gerrit-CC: Victor Ding <victording(a)chromium.org>
Gerrit-MessageType: merged
Attention is currently required from: Edward O'Callaghan, Angel Pons, Daniel Campello.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/51930 )
Change subject: Makefile,meson.build: Fix dependency issues with raiden_debug_spi
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
> No worries, luckily I saw the email in my uncontrollable corp inbox. […]
Yes, a line like this. I couldn't find it.
--
To view, visit https://review.coreboot.org/c/flashrom/+/51930
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I2d6a8c33a2228abf006a9b278bcb7133765c7074
Gerrit-Change-Number: 51930
Gerrit-PatchSet: 1
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Daniel Campello <campello(a)chromium.org>
Gerrit-Comment-Date: Thu, 01 Apr 2021 10:09:45 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Peter Marheine.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/51967 )
Change subject: lspcon_i2c_spi: support a devpath option
......................................................................
Patch Set 2: Code-Review+1
(5 comments)
File lspcon_i2c_spi.c:
https://review.coreboot.org/c/flashrom/+/51967/comment/c7b42129_979dcfe0
PS1, Line 486: msg_perr("%s: one of devpath or bus must be specified\n", __func__);
> Revised further so invalid bus values are distinct from unspecified, allowing the two options to be […]
Looks pretty good, thanks!
File lspcon_i2c_spi.c:
https://review.coreboot.org/c/flashrom/+/51967/comment/1d761af6_038e166e
PS2, Line 457: __func__);
nit: `__func__` probably fits on the previous line
https://review.coreboot.org/c/flashrom/+/51967/comment/1ed569e8_6344b581
PS2, Line 463: __func__);
nit: `__func__` most likely fits on the previous line
https://review.coreboot.org/c/flashrom/+/51967/comment/595e213a_008dcb85
PS2, Line 489:
nit: replace 8 spaces with a tab
https://review.coreboot.org/c/flashrom/+/51967/comment/9d92aef5_92a9d149
PS2, Line 492: } else if (device_path != NULL) {
The first two cases are error paths that return an error, whereas the last two cases are the "normal" paths. I wouldn't use `else` with the former, in order to differentiate both kinds of paths:
if (device_path != NULL && bus_number >= 0) {
msg_perr("%s: only one of bus and devpath may be specified\n", __func__);
free(device_path);
return SPI_GENERIC_ERROR;
}
if (device_path == NULL && bus_number < 0) {
msg_perr("%s: one of bus and devpath must be specified\n", __func__);
return SPI_GENERIC_ERROR;
}
if (device_path != NULL) {
fd = i2c_open_path(device_path, REGISTER_ADDRESS, 0);
free(device_path);
} else {
fd = i2c_open(bus_number, REGISTER_ADDRESS, 0);
}
--
To view, visit https://review.coreboot.org/c/flashrom/+/51967
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Id2adf8a307b9205ce5e5804a6c3e22f19d0c34c9
Gerrit-Change-Number: 51967
Gerrit-PatchSet: 2
Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Daniel Campello <campello(a)chromium.org>
Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Comment-Date: Thu, 01 Apr 2021 07:35:07 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Comment-In-Reply-To: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: David Hendricks, Edward O'Callaghan.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/51243 )
Change subject: Enable dynamic memory allocation checks for cmocka unit tests
......................................................................
Patch Set 7:
(1 comment)
Patchset:
PS7:
I wanted to say thank you to all my reviewers! I see +2s so if you think this can be submitted, could you please submit? (I can’t do it myself).
--
To view, visit https://review.coreboot.org/c/flashrom/+/51243
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I0c6b6b8dc17aaee28640e3fca3d1fc9f7feabf5f
Gerrit-Change-Number: 51243
Gerrit-PatchSet: 7
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Daniel Kurtz <djkurtz(a)google.com>
Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Daniel Kurtz <djkurtz(a)chromium.org>
Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-CC: Thomas Heijligen <src(a)posteo.de>
Gerrit-CC: Victor Ding <victording(a)chromium.org>
Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Comment-Date: Thu, 01 Apr 2021 01:59:28 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: David Hendricks, Edward O'Callaghan.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/51243 )
Change subject: Enable dynamic memory allocation checks for cmocka unit tests
......................................................................
Patch Set 7:
(1 comment)
File meson.build:
https://review.coreboot.org/c/flashrom/+/51243/comment/af2e565f_3972543d
PS6, Line 477: -include
> Ah, sorry. I'm still very used to the comfort and pain of having […]
I am fine with no-spaces, it aligns with -D definitions (like the ones above -DCONFIG_DEFAULT_PROGRAMMER_ARGS="" there is no space between D and the rest).
--
To view, visit https://review.coreboot.org/c/flashrom/+/51243
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I0c6b6b8dc17aaee28640e3fca3d1fc9f7feabf5f
Gerrit-Change-Number: 51243
Gerrit-PatchSet: 7
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Daniel Kurtz <djkurtz(a)google.com>
Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Daniel Kurtz <djkurtz(a)chromium.org>
Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-CC: Thomas Heijligen <src(a)posteo.de>
Gerrit-CC: Victor Ding <victording(a)chromium.org>
Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Comment-Date: Thu, 01 Apr 2021 00:49:47 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Angel Pons.
Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/51967 )
Change subject: lspcon_i2c_spi: support a devpath option
......................................................................
Patch Set 2:
(4 comments)
File i2c_helper_linux.c:
https://review.coreboot.org/c/flashrom/+/51967/comment/f3f869b9_d6696f3e
PS1, Line 46: goto err;
> `goto` isn't necessary here, just return `fd`.
Done
https://review.coreboot.org/c/flashrom/+/51967/comment/bd0b9247_22fa4da3
PS1, Line 54: goto err;
> Same here, just return `err`.
Done
https://review.coreboot.org/c/flashrom/+/51967/comment/ea7c850c_6bde3ec2
PS1, Line 58: return ret ? ret : fd;
> And return `fd` here.
Done
File lspcon_i2c_spi.c:
https://review.coreboot.org/c/flashrom/+/51967/comment/0b3378f0_060292fc
PS1, Line 486: msg_perr("%s: one of devpath or bus must be specified\n", __func__);
> If both arguments are specified, `bus` is silently ignored. […]
Revised further so invalid bus values are distinct from unspecified, allowing the two options to be mutually exclusive.
--
To view, visit https://review.coreboot.org/c/flashrom/+/51967
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Id2adf8a307b9205ce5e5804a6c3e22f19d0c34c9
Gerrit-Change-Number: 51967
Gerrit-PatchSet: 2
Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Daniel Campello <campello(a)chromium.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Thu, 01 Apr 2021 00:12:15 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-MessageType: comment
Hello build bot (Jenkins), Edward O'Callaghan, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/51967
to look at the new patch set (#2).
Change subject: lspcon_i2c_spi: support a devpath option
......................................................................
lspcon_i2c_spi: support a devpath option
Some callers may find it easier to provide the path to an I2C device
at which to communicate with the device, rather than specify the bus
number- doing so might involve trying to parse a path to extract the
number only for flashrom to do the reverse, which is error-prone and
unnecessary.
This change adds support for a `devpath` option, continuing to
allow `bus` and requiring only one of them be specified.
TEST=Verified --flash-size outputs correct values with both
devpath=/dev/i2c-7 and bus=7, as well as noting that one is
required if neither is specified and only one may be specified
if both are given.
Signed-off-by: Peter Marheine <pmarheine(a)chromium.org>
Change-Id: Id2adf8a307b9205ce5e5804a6c3e22f19d0c34c9
---
M i2c_helper.h
M i2c_helper_linux.c
M lspcon_i2c_spi.c
3 files changed, 79 insertions(+), 50 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/67/51967/2
--
To view, visit https://review.coreboot.org/c/flashrom/+/51967
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Id2adf8a307b9205ce5e5804a6c3e22f19d0c34c9
Gerrit-Change-Number: 51967
Gerrit-PatchSet: 2
Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Daniel Campello <campello(a)chromium.org>
Gerrit-MessageType: newpatchset