Hello Patrick Georgi, Aaron Durbin,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/41118
to review the following change.
Change subject: cbfstool: Rename cbfs_walk() to cbfs_legacy_walk() ......................................................................
cbfstool: Rename cbfs_walk() to cbfs_legacy_walk()
This function name clashes with cbfs_walk() in the new commonlib CBFS stack, so rename it to cbfs_legacy_walk(). While we could replace it with the new commonlib implementation, it still has support for certain features in the deprecated pre-FMAP CBFSes (such as non-standard header alignment), which are needed to handle old files but probably not something we'd want to burden the commonlib implementation with. So until we decide to deprecate support for those files from cbfstool as well, it seems easier to just keep the existing implementation here.
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: I37c7e7aa9a206372817d8d0b8f66d72bafb4f346 --- M util/cbfstool/cbfs_image.c M util/cbfstool/cbfs_image.h 2 files changed, 13 insertions(+), 11 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/18/41118/1
diff --git a/util/cbfstool/cbfs_image.c b/util/cbfstool/cbfs_image.c index 035b196..7942b27 100644 --- a/util/cbfstool/cbfs_image.c +++ b/util/cbfstool/cbfs_image.c @@ -469,7 +469,7 @@ cbfs_create_empty_entry(entry, CBFS_TYPE_NULL, last_entry_size, ""); /* If the last entry was an empty file, merge them. */ - cbfs_walk(&image, cbfs_merge_empty_entry, NULL); + cbfs_legacy_walk(&image, cbfs_merge_empty_entry, NULL); }
return 0; @@ -621,7 +621,7 @@ prev_size, "");
/* Merge any potential empty entries together. */ - cbfs_walk(image, cbfs_merge_empty_entry, NULL); + cbfs_legacy_walk(image, cbfs_merge_empty_entry, NULL);
/* * Since current switched to an empty file keep track of it. @@ -760,7 +760,7 @@
// Merge empty entries. DEBUG("(trying to merge empty entries...)\n"); - cbfs_walk(image, cbfs_merge_empty_entry, NULL); + cbfs_legacy_walk(image, cbfs_merge_empty_entry, NULL);
for (entry = cbfs_find_first_entry(image); entry && cbfs_is_valid_entry(image, entry); @@ -1380,7 +1380,7 @@ DEBUG("cbfs_remove_entry: Removed %s @ 0x%x\n", entry->filename, cbfs_get_entry_addr(image, entry)); entry->type = htonl(CBFS_TYPE_DELETED); - cbfs_walk(image, cbfs_merge_empty_entry, NULL); + cbfs_legacy_walk(image, cbfs_merge_empty_entry, NULL); return 0; }
@@ -1597,7 +1597,7 @@ if (cbfs_is_legacy_cbfs(image)) cbfs_print_header_info(image); printf("%-30s %-10s %-12s Size Comp\n", "Name", "Offset", "Type"); - cbfs_walk(image, cbfs_print_entry_info, NULL); + cbfs_legacy_walk(image, cbfs_print_entry_info, NULL); return 0; }
@@ -1617,7 +1617,7 @@ for (i = 0; i < ARRAY_SIZE(header) - 1; i++) fprintf(stdout, "%s%s", header[i], sep); fprintf(stdout, "%s\n", header[i]); - cbfs_walk(image, cbfs_print_parseable_entry_info, stdout); + cbfs_legacy_walk(image, cbfs_print_parseable_entry_info, stdout); return 0; }
@@ -1660,7 +1660,7 @@ return 0; }
-int cbfs_walk(struct cbfs_image *image, cbfs_entry_callback callback, +int cbfs_legacy_walk(struct cbfs_image *image, cbfs_entry_callback callback, void *arg) { int count = 0; @@ -1970,7 +1970,7 @@ need_len = metadata_size + size;
// Merge empty entries to build get max available space. - cbfs_walk(image, cbfs_merge_empty_entry, NULL); + cbfs_legacy_walk(image, cbfs_merge_empty_entry, NULL);
/* Three cases of content location on memory page: * case 1. diff --git a/util/cbfstool/cbfs_image.h b/util/cbfstool/cbfs_image.h index 6ff0746..a3a6fb9 100644 --- a/util/cbfstool/cbfs_image.h +++ b/util/cbfstool/cbfs_image.h @@ -129,16 +129,18 @@ int32_t cbfs_locate_entry(struct cbfs_image *image, size_t size, size_t page_size, size_t align, size_t metadata_size);
-/* Callback function used by cbfs_walk. +/* Callback function used by cbfs_legacy_walk. * Returns 0 on success, or non-zero to stop further iteration. */ typedef int (*cbfs_entry_callback)(struct cbfs_image *image, struct cbfs_file *file, void *arg);
/* Iterates through all entries in CBFS image, and invoke with callback. - * Stops if callback returns non-zero values. + * Stops if callback returns non-zero values. Unlike the commonlib cbfs_walk(), + * this can deal with different alignments in legacy CBFS (with master header). * Returns number of entries invoked. */ -int cbfs_walk(struct cbfs_image *image, cbfs_entry_callback callback, void *arg); +int cbfs_legacy_walk(struct cbfs_image *image, cbfs_entry_callback callback, + void *arg);
/* Primitive CBFS utilities */
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41118 )
Change subject: cbfstool: Rename cbfs_walk() to cbfs_legacy_walk() ......................................................................
Patch Set 5: Code-Review+2
Julius Werner has submitted this change. ( https://review.coreboot.org/c/coreboot/+/41118 )
Change subject: cbfstool: Rename cbfs_walk() to cbfs_legacy_walk() ......................................................................
cbfstool: Rename cbfs_walk() to cbfs_legacy_walk()
This function name clashes with cbfs_walk() in the new commonlib CBFS stack, so rename it to cbfs_legacy_walk(). While we could replace it with the new commonlib implementation, it still has support for certain features in the deprecated pre-FMAP CBFSes (such as non-standard header alignment), which are needed to handle old files but probably not something we'd want to burden the commonlib implementation with. So until we decide to deprecate support for those files from cbfstool as well, it seems easier to just keep the existing implementation here.
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: I37c7e7aa9a206372817d8d0b8f66d72bafb4f346 Reviewed-on: https://review.coreboot.org/c/coreboot/+/41118 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Aaron Durbin adurbin@chromium.org --- M util/cbfstool/cbfs_image.c M util/cbfstool/cbfs_image.h 2 files changed, 13 insertions(+), 11 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved
diff --git a/util/cbfstool/cbfs_image.c b/util/cbfstool/cbfs_image.c index 793713f..5d95814 100644 --- a/util/cbfstool/cbfs_image.c +++ b/util/cbfstool/cbfs_image.c @@ -454,7 +454,7 @@ cbfs_create_empty_entry(entry, CBFS_TYPE_NULL, last_entry_size, ""); /* If the last entry was an empty file, merge them. */ - cbfs_walk(&image, cbfs_merge_empty_entry, NULL); + cbfs_legacy_walk(&image, cbfs_merge_empty_entry, NULL); }
return 0; @@ -606,7 +606,7 @@ prev_size, "");
/* Merge any potential empty entries together. */ - cbfs_walk(image, cbfs_merge_empty_entry, NULL); + cbfs_legacy_walk(image, cbfs_merge_empty_entry, NULL);
/* * Since current switched to an empty file keep track of it. @@ -745,7 +745,7 @@
// Merge empty entries. DEBUG("(trying to merge empty entries...)\n"); - cbfs_walk(image, cbfs_merge_empty_entry, NULL); + cbfs_legacy_walk(image, cbfs_merge_empty_entry, NULL);
for (entry = cbfs_find_first_entry(image); entry && cbfs_is_valid_entry(image, entry); @@ -1365,7 +1365,7 @@ DEBUG("cbfs_remove_entry: Removed %s @ 0x%x\n", entry->filename, cbfs_get_entry_addr(image, entry)); entry->type = htonl(CBFS_TYPE_DELETED); - cbfs_walk(image, cbfs_merge_empty_entry, NULL); + cbfs_legacy_walk(image, cbfs_merge_empty_entry, NULL); return 0; }
@@ -1582,7 +1582,7 @@ if (cbfs_is_legacy_cbfs(image)) cbfs_print_header_info(image); printf("%-30s %-10s %-12s Size Comp\n", "Name", "Offset", "Type"); - cbfs_walk(image, cbfs_print_entry_info, NULL); + cbfs_legacy_walk(image, cbfs_print_entry_info, NULL); return 0; }
@@ -1602,7 +1602,7 @@ for (i = 0; i < ARRAY_SIZE(header) - 1; i++) fprintf(stdout, "%s%s", header[i], sep); fprintf(stdout, "%s\n", header[i]); - cbfs_walk(image, cbfs_print_parseable_entry_info, stdout); + cbfs_legacy_walk(image, cbfs_print_parseable_entry_info, stdout); return 0; }
@@ -1645,7 +1645,7 @@ return 0; }
-int cbfs_walk(struct cbfs_image *image, cbfs_entry_callback callback, +int cbfs_legacy_walk(struct cbfs_image *image, cbfs_entry_callback callback, void *arg) { int count = 0; @@ -1955,7 +1955,7 @@ need_len = metadata_size + size;
// Merge empty entries to build get max available space. - cbfs_walk(image, cbfs_merge_empty_entry, NULL); + cbfs_legacy_walk(image, cbfs_merge_empty_entry, NULL);
/* Three cases of content location on memory page: * case 1. diff --git a/util/cbfstool/cbfs_image.h b/util/cbfstool/cbfs_image.h index cd44438..74c35c9 100644 --- a/util/cbfstool/cbfs_image.h +++ b/util/cbfstool/cbfs_image.h @@ -117,16 +117,18 @@ int32_t cbfs_locate_entry(struct cbfs_image *image, size_t size, size_t page_size, size_t align, size_t metadata_size);
-/* Callback function used by cbfs_walk. +/* Callback function used by cbfs_legacy_walk. * Returns 0 on success, or non-zero to stop further iteration. */ typedef int (*cbfs_entry_callback)(struct cbfs_image *image, struct cbfs_file *file, void *arg);
/* Iterates through all entries in CBFS image, and invoke with callback. - * Stops if callback returns non-zero values. + * Stops if callback returns non-zero values. Unlike the commonlib cbfs_walk(), + * this can deal with different alignments in legacy CBFS (with master header). * Returns number of entries invoked. */ -int cbfs_walk(struct cbfs_image *image, cbfs_entry_callback callback, void *arg); +int cbfs_legacy_walk(struct cbfs_image *image, cbfs_entry_callback callback, + void *arg);
/* Primitive CBFS utilities */
-
Aaron Durbin (Code Review) -
Julius Werner (Code Review)