Change in ...flashrom[master]: layout.c: Don't use global variables for included regions
Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/31247
Change subject: layout.c: Don't use global variables for included regions ......................................................................
layout.c: Don't use global variables for included regions
This removes the use of global variables for included region arguments and also uses a linked list to store the arguments.
Change-Id: I6534cc58b8dcc6256c2730c809286d8083669a6c Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M cli_classic.c M flash.h M layout.c M layout.h 4 files changed, 55 insertions(+), 52 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/47/31247/1
diff --git a/cli_classic.c b/cli_classic.c index ced08c6..5ef8077 100644 --- a/cli_classic.c +++ b/cli_classic.c @@ -151,6 +151,7 @@ #endif /* !STANDALONE */ char *tempstr = NULL; char *pparam = NULL; + struct layout_include_args *include_args = NULL;
flashrom_set_log_callback((flashrom_log_callback *)&flashrom_print_cb);
@@ -290,7 +291,7 @@ break; case 'i': tempstr = strdup(optarg); - if (register_include_arg(tempstr)) { + if (register_include_arg(&include_args, tempstr)) { free(tempstr); cli_classic_abort_usage(); } @@ -452,7 +453,7 @@ goto out; }
- if (!ifd && !fmap && process_include_args(get_global_layout())) { + if (!ifd && !fmap && process_include_args(include_args, get_global_layout())) { ret = 1; goto out; } @@ -608,7 +609,7 @@ if (layoutfile) { layout = get_global_layout(); } else if (ifd && (flashrom_layout_read_from_ifd(&layout, fill_flash, NULL, 0) || - process_include_args(layout))) { + process_include_args(include_args, layout))) { ret = 1; goto out_shutdown; } else if (fmap && fmapfile) { @@ -633,14 +634,14 @@ }
if (flashrom_layout_read_fmap_from_buffer(&layout, fill_flash, fmapfile_buffer, fmapfile_size) || - process_include_args(layout)) { + process_include_args(include_args, layout)) { ret = 1; free(fmapfile_buffer); goto out_shutdown; } free(fmapfile_buffer); } else if (fmap && (flashrom_layout_read_fmap_from_rom(&layout, fill_flash, 0, - fill_flash->chip->total_size * 1024) || process_include_args(layout))) { + fill_flash->chip->total_size * 1024) || process_include_args(include_args, layout))) { ret = 1; goto out_shutdown; } @@ -675,7 +676,7 @@ for (i = 0; i < chipcount; i++) free(flashes[i].chip);
- layout_cleanup(); + layout_cleanup(&include_args); free(filename); free(fmapfile); free(referencefile); diff --git a/flash.h b/flash.h index 911214c..5bbfa0a 100644 --- a/flash.h +++ b/flash.h @@ -391,10 +391,10 @@ #define msg_cspew(...) print(FLASHROM_MSG_SPEW, __VA_ARGS__) /* chip debug spew */
/* layout.c */ -int register_include_arg(char *name); +int register_include_arg(struct layout_include_args **args, char *name); int read_romlayout(const char *name); int normalize_romentries(const struct flashctx *flash); -void layout_cleanup(void); +void layout_cleanup(struct layout_include_args **args);
/* spi.c */ struct spi_command { diff --git a/layout.c b/layout.c index 384a513..583c733 100644 --- a/layout.c +++ b/layout.c @@ -26,11 +26,6 @@ struct romentry entries[MAX_ROMLAYOUT]; static struct flashrom_layout layout = { entries, 0 };
-/* include_args holds the arguments specified at the command line with -i. They must be processed at some point - * so that desired regions are marked as "included" in the layout. */ -static char *include_args[MAX_ROMLAYOUT]; -static int num_include_args = 0; /* the number of valid include_args. */ - struct flashrom_layout *get_global_layout(void) { return &layout; @@ -101,37 +96,30 @@ } #endif
-/* returns the index of the entry (or a negative value if it is not found) */ -static int find_include_arg(const char *const name) -{ - unsigned int i; - for (i = 0; i < num_include_args; i++) { - if (!strcmp(include_args[i], name)) - return i; - } - return -1; -} - /* register an include argument (-i) for later processing */ -int register_include_arg(char *name) +int register_include_arg(struct layout_include_args **args, char *name) { - if (num_include_args >= MAX_ROMLAYOUT) { - msg_gerr("Too many regions included (%i).\n", num_include_args); - return 1; - } - + struct layout_include_args *tmp; if (name == NULL) { msg_gerr("<NULL> is a bad region name.\n"); return 1; }
- if (find_include_arg(name) != -1) { - msg_gerr("Duplicate region name: "%s".\n", name); - return 1; + tmp = *args; + while(tmp) { + if (!strcmp(tmp->name, name)) { + msg_gerr("Duplicate region name: "%s".\n", name); + return 1; + } + tmp = tmp->next; }
- include_args[num_include_args] = name; - num_include_args++; + tmp = malloc(sizeof(struct layout_include_args)); + + tmp->name = name; + tmp->next = *args; + *args = tmp; + return 0; }
@@ -153,45 +141,54 @@ /* process -i arguments * returns 0 to indicate success, >0 to indicate failure */ -int process_include_args(struct flashrom_layout *const l) +int process_include_args(const struct layout_include_args * const args, struct flashrom_layout *const l) { - int i; + int found = 0; + const struct layout_include_args *tmp;
- if (num_include_args == 0) + if (args == NULL) return 0;
/* User has specified an area, but no layout file is loaded. */ if (l->num_entries == 0) { msg_gerr("Region requested (with -i "%s"), " "but no layout data is available.\n", - include_args[0]); + args->name); return 1; }
- for (i = 0; i < num_include_args; i++) { - if (find_romentry(l, include_args[i]) < 0) { + tmp = args; + while (tmp) { + if (find_romentry(l, tmp->name) < 0) { msg_gerr("Invalid region specified: "%s".\n", - include_args[i]); + tmp->name); return 1; } + tmp = tmp->next; + found++; }
- msg_ginfo("Using region%s: "%s"", num_include_args > 1 ? "s" : "", - include_args[0]); - for (i = 1; i < num_include_args; i++) - msg_ginfo(", "%s"", include_args[i]); + msg_ginfo("Using region%s:", found > 1 ? "s" : ""); + tmp = args; + while (tmp) { + msg_ginfo(" "%s"%s", tmp->name, found > 1 ? "," : ""); + found--; + tmp = tmp->next; + } msg_ginfo(".\n"); return 0; }
-void layout_cleanup(void) +void layout_cleanup(struct layout_include_args **args) { int i; - for (i = 0; i < num_include_args; i++) { - free(include_args[i]); - include_args[i] = NULL; + struct layout_include_args *tmp; + + while (*args) { + tmp = (*args)->next; + free(*args); + *args = tmp; } - num_include_args = 0;
for (i = 0; i < layout.num_entries; i++) { layout.entries[i].included = 0; diff --git a/layout.h b/layout.h index eb54a4f..63607a3 100644 --- a/layout.h +++ b/layout.h @@ -53,10 +53,15 @@ struct romentry entry; };
+struct layout_include_args { + char *name; + struct layout_include_args *next; +}; + struct flashrom_layout *get_global_layout(void); struct flashrom_flashctx; const struct flashrom_layout *get_layout(const struct flashrom_flashctx *const flashctx);
-int process_include_args(struct flashrom_layout *); +int process_include_args(const struct layout_include_args * const args, struct flashrom_layout *const l);
#endif /* !__LAYOUT_H__ */
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/31247 )
Change subject: layout.c: Don't use global variables for included regions ......................................................................
Patch Set 1: Code-Review+1
(6 comments)
Just style comments...
https://review.coreboot.org/#/c/31247/1/layout.h File layout.h:
https://review.coreboot.org/#/c/31247/1/layout.h@65 PS1, Line 65: const The latter `const` should be dropped (caller doesn't care if it's const inside the implementation).
https://review.coreboot.org/#/c/31247/1/layout.h@65 PS1, Line 65: int process_include_args(const struct layout_include_args * const args, struct flashrom_layout *const l); I would prefer to have the output parameter first.
https://review.coreboot.org/#/c/31247/1/layout.h@65 PS1, Line 65: const same
https://review.coreboot.org/#/c/31247/1/layout.c File layout.c:
https://review.coreboot.org/#/c/31247/1/layout.c@109 PS1, Line 109: while(tmp) { missing space
https://review.coreboot.org/#/c/31247/1/layout.c@117 PS1, Line 117: tmp = malloc(sizeof(struct layout_include_args)); null-check
https://review.coreboot.org/#/c/31247/1/layout.c@144 PS1, Line 144: spurious space between `*` and `const`
Hello build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/31247
to look at the new patch set (#2).
Change subject: layout.c: Don't use global variables for included regions ......................................................................
layout.c: Don't use global variables for included regions
This removes the use of global variables for included region arguments and also uses a linked list to store the arguments.
Change-Id: I6534cc58b8dcc6256c2730c809286d8083669a6c Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M cli_classic.c M flash.h M layout.c M layout.h 4 files changed, 58 insertions(+), 51 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/47/31247/2
Hello build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/31247
to look at the new patch set (#3).
Change subject: layout.c: Don't use global variables for included regions ......................................................................
layout.c: Don't use global variables for included regions
This removes the use of global variables for included region arguments and also uses a linked list to store the arguments.
Change-Id: I6534cc58b8dcc6256c2730c809286d8083669a6c Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M cli_classic.c M flash.h M layout.c M layout.h 4 files changed, 58 insertions(+), 51 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/47/31247/3
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/31247 )
Change subject: layout.c: Don't use global variables for included regions ......................................................................
Patch Set 5:
This change is ready for review.
Hello Angel Pons, Thomas Heijligen, David Hendricks, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/31247
to look at the new patch set (#6).
Change subject: layout.c: Don't use global variables for included regions ......................................................................
layout.c: Don't use global variables for included regions
This removes the use of global variables for included region arguments and also uses a linked list to store the arguments.
Change-Id: I6534cc58b8dcc6256c2730c809286d8083669a6c Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M cli_classic.c M flash.h M layout.c M layout.h 4 files changed, 58 insertions(+), 51 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/47/31247/6
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/31247 )
Change subject: layout.c: Don't use global variables for included regions ......................................................................
Patch Set 6: Code-Review+2
(5 comments)
https://review.coreboot.org/#/c/31247/1/layout.h File layout.h:
https://review.coreboot.org/#/c/31247/1/layout.h@65 PS1, Line 65: const
same
Done
https://review.coreboot.org/#/c/31247/1/layout.h@65 PS1, Line 65: int process_include_args(const struct layout_include_args * const args, struct flashrom_layout *const l);
I would prefer to have the output parameter first.
Done
https://review.coreboot.org/#/c/31247/1/layout.c File layout.c:
https://review.coreboot.org/#/c/31247/1/layout.c@109 PS1, Line 109: while(tmp) {
missing space
Done
https://review.coreboot.org/#/c/31247/1/layout.c@117 PS1, Line 117: tmp = malloc(sizeof(struct layout_include_args));
null-check
Done
https://review.coreboot.org/#/c/31247/1/layout.c@144 PS1, Line 144:
spurious space between `*` and `const`
Done
Nico Huber has submitted this change and it was merged. ( https://review.coreboot.org/c/flashrom/+/31247 )
Change subject: layout.c: Don't use global variables for included regions ......................................................................
layout.c: Don't use global variables for included regions
This removes the use of global variables for included region arguments and also uses a linked list to store the arguments.
Change-Id: I6534cc58b8dcc6256c2730c809286d8083669a6c Signed-off-by: Arthur Heymans arthur@aheymans.xyz Reviewed-on: https://review.coreboot.org/c/flashrom/+/31247 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Nico Huber nico.h@gmx.de --- M cli_classic.c M flash.h M layout.c M layout.h 4 files changed, 58 insertions(+), 51 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved
diff --git a/cli_classic.c b/cli_classic.c index ced08c6..2e07612 100644 --- a/cli_classic.c +++ b/cli_classic.c @@ -151,6 +151,7 @@ #endif /* !STANDALONE */ char *tempstr = NULL; char *pparam = NULL; + struct layout_include_args *include_args = NULL;
flashrom_set_log_callback((flashrom_log_callback *)&flashrom_print_cb);
@@ -290,7 +291,7 @@ break; case 'i': tempstr = strdup(optarg); - if (register_include_arg(tempstr)) { + if (register_include_arg(&include_args, tempstr)) { free(tempstr); cli_classic_abort_usage(); } @@ -452,7 +453,7 @@ goto out; }
- if (!ifd && !fmap && process_include_args(get_global_layout())) { + if (!ifd && !fmap && process_include_args(get_global_layout(), include_args)) { ret = 1; goto out; } @@ -608,7 +609,7 @@ if (layoutfile) { layout = get_global_layout(); } else if (ifd && (flashrom_layout_read_from_ifd(&layout, fill_flash, NULL, 0) || - process_include_args(layout))) { + process_include_args(layout, include_args))) { ret = 1; goto out_shutdown; } else if (fmap && fmapfile) { @@ -633,14 +634,14 @@ }
if (flashrom_layout_read_fmap_from_buffer(&layout, fill_flash, fmapfile_buffer, fmapfile_size) || - process_include_args(layout)) { + process_include_args(layout, include_args)) { ret = 1; free(fmapfile_buffer); goto out_shutdown; } free(fmapfile_buffer); } else if (fmap && (flashrom_layout_read_fmap_from_rom(&layout, fill_flash, 0, - fill_flash->chip->total_size * 1024) || process_include_args(layout))) { + fill_flash->chip->total_size * 1024) || process_include_args(layout, include_args))) { ret = 1; goto out_shutdown; } @@ -675,7 +676,7 @@ for (i = 0; i < chipcount; i++) free(flashes[i].chip);
- layout_cleanup(); + layout_cleanup(&include_args); free(filename); free(fmapfile); free(referencefile); diff --git a/flash.h b/flash.h index 911214c..5bbfa0a 100644 --- a/flash.h +++ b/flash.h @@ -391,10 +391,10 @@ #define msg_cspew(...) print(FLASHROM_MSG_SPEW, __VA_ARGS__) /* chip debug spew */
/* layout.c */ -int register_include_arg(char *name); +int register_include_arg(struct layout_include_args **args, char *name); int read_romlayout(const char *name); int normalize_romentries(const struct flashctx *flash); -void layout_cleanup(void); +void layout_cleanup(struct layout_include_args **args);
/* spi.c */ struct spi_command { diff --git a/layout.c b/layout.c index f757783..52a0c2e 100644 --- a/layout.c +++ b/layout.c @@ -26,11 +26,6 @@ struct romentry entries[MAX_ROMLAYOUT]; static struct flashrom_layout layout = { entries, 0 };
-/* include_args holds the arguments specified at the command line with -i. They must be processed at some point - * so that desired regions are marked as "included" in the layout. */ -static char *include_args[MAX_ROMLAYOUT]; -static int num_include_args = 0; /* the number of valid include_args. */ - struct flashrom_layout *get_global_layout(void) { return &layout; @@ -101,37 +96,34 @@ } #endif
-/* returns the index of the entry (or a negative value if it is not found) */ -static int find_include_arg(const char *const name) -{ - unsigned int i; - for (i = 0; i < num_include_args; i++) { - if (!strcmp(include_args[i], name)) - return i; - } - return -1; -} - /* register an include argument (-i) for later processing */ -int register_include_arg(char *name) +int register_include_arg(struct layout_include_args **args, char *name) { - if (num_include_args >= MAX_ROMLAYOUT) { - msg_gerr("Too many regions included (%i).\n", num_include_args); - return 1; - } - + struct layout_include_args *tmp; if (name == NULL) { msg_gerr("<NULL> is a bad region name.\n"); return 1; }
- if (find_include_arg(name) != -1) { - msg_gerr("Duplicate region name: "%s".\n", name); + tmp = *args; + while (tmp) { + if (!strcmp(tmp->name, name)) { + msg_gerr("Duplicate region name: "%s".\n", name); + return 1; + } + tmp = tmp->next; + } + + tmp = malloc(sizeof(struct layout_include_args)); + if (tmp == NULL) { + msg_gerr("Could not allocate memory"); return 1; }
- include_args[num_include_args] = name; - num_include_args++; + tmp->name = name; + tmp->next = *args; + *args = tmp; + return 0; }
@@ -153,45 +145,54 @@ /* process -i arguments * returns 0 to indicate success, >0 to indicate failure */ -int process_include_args(struct flashrom_layout *const l) +int process_include_args(struct flashrom_layout *l, const struct layout_include_args *const args) { - int i; + int found = 0; + const struct layout_include_args *tmp;
- if (num_include_args == 0) + if (args == NULL) return 0;
/* User has specified an area, but no layout file is loaded. */ if (l->num_entries == 0) { msg_gerr("Region requested (with -i "%s"), " "but no layout data is available.\n", - include_args[0]); + args->name); return 1; }
- for (i = 0; i < num_include_args; i++) { - if (find_romentry(l, include_args[i]) < 0) { + tmp = args; + while (tmp) { + if (find_romentry(l, tmp->name) < 0) { msg_gerr("Invalid region specified: "%s".\n", - include_args[i]); + tmp->name); return 1; } + tmp = tmp->next; + found++; }
- msg_ginfo("Using region%s: "%s"", num_include_args > 1 ? "s" : "", - include_args[0]); - for (i = 1; i < num_include_args; i++) - msg_ginfo(", "%s"", include_args[i]); + msg_ginfo("Using region%s:", found > 1 ? "s" : ""); + tmp = args; + while (tmp) { + msg_ginfo(" "%s"%s", tmp->name, found > 1 ? "," : ""); + found--; + tmp = tmp->next; + } msg_ginfo(".\n"); return 0; }
-void layout_cleanup(void) +void layout_cleanup(struct layout_include_args **args) { int i; - for (i = 0; i < num_include_args; i++) { - free(include_args[i]); - include_args[i] = NULL; + struct layout_include_args *tmp; + + while (*args) { + tmp = (*args)->next; + free(*args); + *args = tmp; } - num_include_args = 0;
for (i = 0; i < layout.num_entries; i++) { layout.entries[i].included = 0; diff --git a/layout.h b/layout.h index 5c07407..0f07e09 100644 --- a/layout.h +++ b/layout.h @@ -54,11 +54,16 @@ struct romentry entry; };
+struct layout_include_args { + char *name; + struct layout_include_args *next; +}; + struct flashrom_layout *get_global_layout(void); struct flashrom_flashctx; const struct flashrom_layout *get_layout(const struct flashrom_flashctx *const flashctx);
-int process_include_args(struct flashrom_layout *); +int process_include_args(struct flashrom_layout *l, const struct layout_include_args *const args); const struct romentry *layout_next_included_region(const struct flashrom_layout *, chipoff_t);
#endif /* !__LAYOUT_H__ */
-
Arthur Heymans (Code Review) -
Nico Huber (Code Review)