Hello Patrick Rudolph,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/32861
to review the following change.
Change subject: device_tree: Switch allocations to xzalloc() ......................................................................
device_tree: Switch allocations to xzalloc()
The FIT code is already using xzalloc() everywhere, and that's the only real consumer of device tree code right now. Chances are if you're trying to unflatten an FDT and it doesn't fit into the heap you're pretty much screwed anyway, so all the OOM handling feels a bit unnecessary (and some functions will just silently fail because they don't have a return value, which is bad). Let's just switch this all to die on failed allocations.
Change-Id: I738f24d550a776653b2becd3d4f7d4d2cb3cc048 Signed-off-by: Julius Werner jwerner@chromium.org --- M src/lib/device_tree.c M src/lib/fit.c 2 files changed, 11 insertions(+), 46 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/61/32861/1
diff --git a/src/lib/device_tree.c b/src/lib/device_tree.c index 8dbc510..bb40eee 100644 --- a/src/lib/device_tree.c +++ b/src/lib/device_tree.c @@ -16,6 +16,7 @@ */
#include <assert.h> +#include <commonlib/stdlib.h> #include <console/console.h> #include <device_tree.h> #include <endian.h> @@ -153,24 +154,6 @@ /* * Functions to turn a flattened tree into an unflattened one. */ -static struct device_tree_node *alloc_node(void) -{ - struct device_tree_node *buf = malloc(sizeof(struct device_tree_node)); - if (!buf) - return NULL; - memset(buf, 0, sizeof(*buf)); - return buf; -} - -static struct device_tree_property *alloc_prop(void) -{ - struct device_tree_property *buf = - malloc(sizeof(struct device_tree_property)); - if (!buf) - return NULL; - memset(buf, 0, sizeof(*buf)); - return buf; -}
static int fdt_unflatten_node(const void *blob, uint32_t start_offset, struct device_tree_node **new_node) @@ -185,18 +168,14 @@ return 0; offset += size;
- struct device_tree_node *node = alloc_node(); + struct device_tree_node *node = xzalloc(sizeof(*node)); *new_node = node; - if (!node) - return 0; node->name = name;
struct fdt_property fprop; last = &node->properties; while ((size = fdt_next_property(blob, offset, &fprop))) { - struct device_tree_property *prop = alloc_prop(); - if (!prop) - return 0; + struct device_tree_property *prop = xzalloc(sizeof(*prop)); prop->prop = fprop;
list_insert_after(&prop->list_node, last); @@ -227,10 +206,7 @@ if (!size) return 0;
- struct device_tree_reserve_map_entry *entry = malloc(sizeof(*entry)); - if (!entry) - return 0; - memset(entry, 0, sizeof(*entry)); + struct device_tree_reserve_map_entry *entry = xzalloc(sizeof(*entry)); *new = entry; entry->start = start; entry->size = size; @@ -240,11 +216,8 @@
struct device_tree *fdt_unflatten(const void *blob) { - struct device_tree *tree = malloc(sizeof(*tree)); + struct device_tree *tree = xzalloc(sizeof(*tree)); const struct fdt_header *header = (const struct fdt_header *)blob; - if (!tree) - return NULL; - memset(tree, 0, sizeof(*tree)); tree->header = header;
uint32_t struct_offset = be32toh(header->structure_offset); @@ -534,7 +507,7 @@ if (!create) return NULL;
- found = alloc_node(); + found = malloc(sizeof(*found)); if (!found) return NULL; found->name = strdup(*path); @@ -804,9 +777,7 @@ } }
- prop = alloc_prop(); - if (!prop) - return; + prop = xzalloc(sizeof(*prop)); list_insert_after(&prop->list_node, &node->properties); prop->prop.name = name; prop->prop.data = data; @@ -878,9 +849,7 @@ */ void dt_add_u32_prop(struct device_tree_node *node, const char *name, u32 val) { - u32 *val_ptr = malloc(sizeof(val)); - if (!val_ptr) - return; + u32 *val_ptr = xmalloc(sizeof(val)); *val_ptr = htobe32(val); dt_add_bin_prop(node, name, val_ptr, sizeof(*val_ptr)); } @@ -894,9 +863,7 @@ */ void dt_add_u64_prop(struct device_tree_node *node, const char *name, u64 val) { - u64 *val_ptr = malloc(sizeof(val)); - if (!val_ptr) - return; + u64 *val_ptr = xmalloc(sizeof(val)); *val_ptr = htobe64(val); dt_add_bin_prop(node, name, val_ptr, sizeof(*val_ptr)); } @@ -916,9 +883,7 @@ { int i; size_t length = (addr_cells + size_cells) * sizeof(u32) * count; - u8 *data = malloc(length); - if (!data) - return; + u8 *data = xmalloc(length); u8 *cur = data;
for (i = 0; i < count; i++) { diff --git a/src/lib/fit.c b/src/lib/fit.c index 3aad806..d15641d 100644 --- a/src/lib/fit.c +++ b/src/lib/fit.c @@ -27,7 +27,7 @@ #include <fit.h> #include <boardid.h> #include <commonlib/cbfs_serialized.h> -#include <commonlib/include/commonlib/stdlib.h> +#include <commonlib/stdlib.h>
static struct list_node image_nodes; static struct list_node config_nodes;
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32861 )
Change subject: device_tree: Switch allocations to xzalloc() ......................................................................
Patch Set 5:
(4 comments)
https://review.coreboot.org/#/c/32861/5/src/lib/device_tree.c File src/lib/device_tree.c:
https://review.coreboot.org/#/c/32861/5/src/lib/device_tree.c@171 PS5, Line 171: xzalloc xmalloc?
https://review.coreboot.org/#/c/32861/5/src/lib/device_tree.c@178 PS5, Line 178: xzalloc xmalloc?
https://review.coreboot.org/#/c/32861/5/src/lib/device_tree.c@510 PS5, Line 510: malloc why not xmalloc?
https://review.coreboot.org/#/c/32861/5/src/lib/device_tree.c@780 PS5, Line 780: xzalloc xmalloc?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32861 )
Change subject: device_tree: Switch allocations to xzalloc() ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/#/c/32861/5/src/lib/device_tree.c File src/lib/device_tree.c:
https://review.coreboot.org/#/c/32861/5/src/lib/device_tree.c@171 PS5, Line 171: xzalloc
xmalloc?
It's just a safety precaution to do this everywhere, I think (copied this from depthcharge). Otherwise you always have to carefully check whether you're *really* initializing every single member, and if anyone ever adds new members then they might forget to change code like this. It's easy to overlook things... for example, if I just changed this to xmalloc() without adding more code, the list_node pointers in the struct wouldn't get initialized to NULL.
(FWIW due to the way the heap in coreboot is currently implemented, allocations are always guaranteed to be zero-initialized anyway. We could optimize xzalloc() to just be a synonym for xmalloc() in coreboot. Not sure if it's worth the risk of someone changing the heap implementation later and not noticing, though. memset() on the L1 cache is cheap.)
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32861 )
Change subject: device_tree: Switch allocations to xzalloc() ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/#/c/32861/5/src/lib/device_tree.c File src/lib/device_tree.c:
https://review.coreboot.org/#/c/32861/5/src/lib/device_tree.c@510 PS5, Line 510: malloc
why not xmalloc?
ok, I can understand that most malloc() here should be xzalloc intead of xmalloc.
But can you clarify why this one is expected to be malloc instead of xzalloc / xmalloc?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32861 )
Change subject: device_tree: Switch allocations to xzalloc() ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/#/c/32861/5/src/lib/device_tree.c File src/lib/device_tree.c:
https://review.coreboot.org/#/c/32861/5/src/lib/device_tree.c@510 PS5, Line 510: malloc
ok, I can understand that most malloc() here should be xzalloc intead of xmalloc. […]
There's no great reason for this one, it's just that depthcharge did it this way and I didn't want to diverge the code unnecessarily (in case we want to pick over more patches later). I think the main point is that we don't have an xstrdup(), so we need to have cases that return NULL for failure anyway, and then you might as well do it for the other allocation in the same function.
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32861 )
Change subject: device_tree: Switch allocations to xzalloc() ......................................................................
Patch Set 5: Code-Review+2
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32861 )
Change subject: device_tree: Switch allocations to xzalloc() ......................................................................
device_tree: Switch allocations to xzalloc()
The FIT code is already using xzalloc() everywhere, and that's the only real consumer of device tree code right now. Chances are if you're trying to unflatten an FDT and it doesn't fit into the heap you're pretty much screwed anyway, so all the OOM handling feels a bit unnecessary (and some functions will just silently fail because they don't have a return value, which is bad). Let's just switch this all to die on failed allocations.
Change-Id: I738f24d550a776653b2becd3d4f7d4d2cb3cc048 Signed-off-by: Julius Werner jwerner@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/32861 Reviewed-by: Hung-Te Lin hungte@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/lib/device_tree.c M src/lib/fit.c 2 files changed, 11 insertions(+), 46 deletions(-)
Approvals: build bot (Jenkins): Verified Hung-Te Lin: Looks good to me, approved
diff --git a/src/lib/device_tree.c b/src/lib/device_tree.c index 8dbc510..bb40eee 100644 --- a/src/lib/device_tree.c +++ b/src/lib/device_tree.c @@ -16,6 +16,7 @@ */
#include <assert.h> +#include <commonlib/stdlib.h> #include <console/console.h> #include <device_tree.h> #include <endian.h> @@ -153,24 +154,6 @@ /* * Functions to turn a flattened tree into an unflattened one. */ -static struct device_tree_node *alloc_node(void) -{ - struct device_tree_node *buf = malloc(sizeof(struct device_tree_node)); - if (!buf) - return NULL; - memset(buf, 0, sizeof(*buf)); - return buf; -} - -static struct device_tree_property *alloc_prop(void) -{ - struct device_tree_property *buf = - malloc(sizeof(struct device_tree_property)); - if (!buf) - return NULL; - memset(buf, 0, sizeof(*buf)); - return buf; -}
static int fdt_unflatten_node(const void *blob, uint32_t start_offset, struct device_tree_node **new_node) @@ -185,18 +168,14 @@ return 0; offset += size;
- struct device_tree_node *node = alloc_node(); + struct device_tree_node *node = xzalloc(sizeof(*node)); *new_node = node; - if (!node) - return 0; node->name = name;
struct fdt_property fprop; last = &node->properties; while ((size = fdt_next_property(blob, offset, &fprop))) { - struct device_tree_property *prop = alloc_prop(); - if (!prop) - return 0; + struct device_tree_property *prop = xzalloc(sizeof(*prop)); prop->prop = fprop;
list_insert_after(&prop->list_node, last); @@ -227,10 +206,7 @@ if (!size) return 0;
- struct device_tree_reserve_map_entry *entry = malloc(sizeof(*entry)); - if (!entry) - return 0; - memset(entry, 0, sizeof(*entry)); + struct device_tree_reserve_map_entry *entry = xzalloc(sizeof(*entry)); *new = entry; entry->start = start; entry->size = size; @@ -240,11 +216,8 @@
struct device_tree *fdt_unflatten(const void *blob) { - struct device_tree *tree = malloc(sizeof(*tree)); + struct device_tree *tree = xzalloc(sizeof(*tree)); const struct fdt_header *header = (const struct fdt_header *)blob; - if (!tree) - return NULL; - memset(tree, 0, sizeof(*tree)); tree->header = header;
uint32_t struct_offset = be32toh(header->structure_offset); @@ -534,7 +507,7 @@ if (!create) return NULL;
- found = alloc_node(); + found = malloc(sizeof(*found)); if (!found) return NULL; found->name = strdup(*path); @@ -804,9 +777,7 @@ } }
- prop = alloc_prop(); - if (!prop) - return; + prop = xzalloc(sizeof(*prop)); list_insert_after(&prop->list_node, &node->properties); prop->prop.name = name; prop->prop.data = data; @@ -878,9 +849,7 @@ */ void dt_add_u32_prop(struct device_tree_node *node, const char *name, u32 val) { - u32 *val_ptr = malloc(sizeof(val)); - if (!val_ptr) - return; + u32 *val_ptr = xmalloc(sizeof(val)); *val_ptr = htobe32(val); dt_add_bin_prop(node, name, val_ptr, sizeof(*val_ptr)); } @@ -894,9 +863,7 @@ */ void dt_add_u64_prop(struct device_tree_node *node, const char *name, u64 val) { - u64 *val_ptr = malloc(sizeof(val)); - if (!val_ptr) - return; + u64 *val_ptr = xmalloc(sizeof(val)); *val_ptr = htobe64(val); dt_add_bin_prop(node, name, val_ptr, sizeof(*val_ptr)); } @@ -916,9 +883,7 @@ { int i; size_t length = (addr_cells + size_cells) * sizeof(u32) * count; - u8 *data = malloc(length); - if (!data) - return; + u8 *data = xmalloc(length); u8 *cur = data;
for (i = 0; i < count; i++) { diff --git a/src/lib/fit.c b/src/lib/fit.c index 3aad806..d15641d 100644 --- a/src/lib/fit.c +++ b/src/lib/fit.c @@ -27,7 +27,7 @@ #include <fit.h> #include <boardid.h> #include <commonlib/cbfs_serialized.h> -#include <commonlib/include/commonlib/stdlib.h> +#include <commonlib/stdlib.h>
static struct list_node image_nodes; static struct list_node config_nodes;
-
Hung-Te Lin (Code Review) -
Julius Werner (Code Review)
-
Patrick Georgi (Code Review)