June 2019 - flashrom-gerrit - mail.coreboot.org

Change in ...flashrom[master]: layout: Never use global `layout` directly
by Angel Pons (Code Review) June 17, 2019

June 17, 2019

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33513 ) Change subject: layout: Never use global `layout` directly ...................................................................... Patch Set 2: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/33513 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ia2d7d1f4f649cd239b559ba6a40ee0977004e774 Gerrit-Change-Number: 33513 Gerrit-PatchSet: 2 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Mon, 17 Jun 2019 08:21:10 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in ...flashrom[master]: layout: Never use global `layout` directly
by Nico Huber (Code Review) June 17, 2019

June 17, 2019

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33513 ) Change subject: layout: Never use global `layout` directly ...................................................................... Patch Set 2: ugh, edited commit message during rebase. lost +2 -- To view, visit https://review.coreboot.org/c/flashrom/+/33513 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ia2d7d1f4f649cd239b559ba6a40ee0977004e774 Gerrit-Change-Number: 33513 Gerrit-PatchSet: 2 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Mon, 17 Jun 2019 08:04:22 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in ...flashrom[master]: layout.c: Don't use global variables for included regions
by Arthur Heymans (Code Review) June 17, 2019

June 17, 2019

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(a)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__ */ -- To view, visit https://review.coreboot.org/c/flashrom/+/31247 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I6534cc58b8dcc6256c2730c809286d8083669a6c Gerrit-Change-Number: 31247 Gerrit-PatchSet: 1 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-MessageType: newchange

2 7

Change in ...flashrom[master]: layout: Use linked list for `struct romentry`
by Nico Huber (Code Review) June 16, 2019

June 16, 2019

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33521 ) Change subject: layout: Use linked list for `struct romentry` ...................................................................... Patch Set 2: (1 comment) https://review.coreboot.org/#/c/33521/2/layout.h File layout.h: https://review.coreboot.org/#/c/33521/2/layout.h@39 PS2, Line 39: struct romentry *next; We should probably hide this in a private wrapper object. Or make the whole struct private and use more functions? -- To view, visit https://review.coreboot.org/c/flashrom/+/33521 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I60a0aa1007ebcd5eb401db116f835d129b3e9732 Gerrit-Change-Number: 33521 Gerrit-PatchSet: 2 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Sun, 16 Jun 2019 18:49:50 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in ...flashrom[master]: layout: Introduce layout_next_included()
by Arthur Heymans (Code Review) June 16, 2019

June 16, 2019

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33518 ) Change subject: layout: Introduce layout_next_included() ...................................................................... Patch Set 3: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/33518 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib01c8af06c3f84eafbd585760e74c3c287b9fa7d Gerrit-Change-Number: 33518 Gerrit-PatchSet: 3 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Sun, 16 Jun 2019 18:43:26 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in ...flashrom[master]: layout: Use linked list for `struct romentry`
by Nico Huber (Code Review) June 16, 2019

June 16, 2019

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33521 ) Change subject: layout: Use linked list for `struct romentry` ...................................................................... Patch Set 2: This change is ready for review. -- To view, visit https://review.coreboot.org/c/flashrom/+/33521 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I60a0aa1007ebcd5eb401db116f835d129b3e9732 Gerrit-Change-Number: 33521 Gerrit-PatchSet: 2 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Sun, 16 Jun 2019 18:26:10 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in ...flashrom[master]: layout: Never use global `layout` directly
by Nico Huber (Code Review) June 16, 2019

June 16, 2019

Hello Angel Pons, Arthur Heymans, Thomas Heijligen, David Hendricks, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/33513 to look at the new patch set (#2). Change subject: layout: Never use global `layout` directly ...................................................................... layout: Never use global `layout` directly And rename it to `global_layout` to free `layout` as a local variable name. We will get rid of the global layout entirely later. Change-Id: Ia2d7d1f4f649cd239b559ba6a40ee0977004e774 Signed-off-by: Nico Huber <nico.h(a)gmx.de> --- M layout.c 1 file changed, 23 insertions(+), 20 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/13/33513/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/33513 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ia2d7d1f4f649cd239b559ba6a40ee0977004e774 Gerrit-Change-Number: 33513 Gerrit-PatchSet: 2 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: newpatchset

1 0

Change in ...flashrom[master]: layout: Introduce get_default_layout()
by Nico Huber (Code Review) June 16, 2019

June 16, 2019

Hello Angel Pons, Arthur Heymans, Thomas Heijligen, David Hendricks, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/33519 to look at the new patch set (#2). Change subject: layout: Introduce get_default_layout() ...................................................................... layout: Introduce get_default_layout() Containing an included, full-flash-chip sized, default region. Change-Id: Ia343e9775ec5bdc3fea5cdb6b347298515996e34 Signed-off-by: Nico Huber <nico.h(a)gmx.de> --- M layout.c M layout.h 2 files changed, 8 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/19/33519/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/33519 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ia343e9775ec5bdc3fea5cdb6b347298515996e34 Gerrit-Change-Number: 33519 Gerrit-PatchSet: 2 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: newpatchset

1 0

Change in ...flashrom[master]: layout: Introduce layout_next_included()
by Nico Huber (Code Review) June 16, 2019

June 16, 2019

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33518 ) Change subject: layout: Introduce layout_next_included() ...................................................................... Patch Set 2: (1 comment) https://review.coreboot.org/#/c/33518/1/layout.c File layout.c: https://review.coreboot.org/#/c/33518/1/layout.c@246 PS1, Line 246: : for (i = 0; i < layout->num_entries; ++i) { : if (!layout->entries[i].included) : continue; : if (!iterator) : return &layout->entries[i]; : if (iterator == &layout->entries[i]) : iterator = NULL; : } > This will result in an endless while-loop that is calling this if there are duplicate entries in the […] It's not comparing entries but their pointers. But this is really ugly code (maybe even too ugly for an interim solution) so I rewrote it. -- To view, visit https://review.coreboot.org/c/flashrom/+/33518 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib01c8af06c3f84eafbd585760e74c3c287b9fa7d Gerrit-Change-Number: 33518 Gerrit-PatchSet: 2 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Sun, 16 Jun 2019 10:44:41 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-MessageType: comment

1 0

Change in ...flashrom[master]: layout: Introduce layout_next_included()
by Nico Huber (Code Review) June 16, 2019

June 16, 2019

Hello Angel Pons, Arthur Heymans, Thomas Heijligen, David Hendricks, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/33518 to look at the new patch set (#2). Change subject: layout: Introduce layout_next_included() ...................................................................... layout: Introduce layout_next_included() Change-Id: Ib01c8af06c3f84eafbd585760e74c3c287b9fa7d Signed-off-by: Nico Huber <nico.h(a)gmx.de> --- M flashrom.c M layout.c M layout.h 3 files changed, 31 insertions(+), 21 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/18/33518/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/33518 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib01c8af06c3f84eafbd585760e74c3c287b9fa7d Gerrit-Change-Number: 33518 Gerrit-PatchSet: 2 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: newpatchset

1 0