December 2022 - flashrom-gerrit

[S] Change in flashrom[master]: README: Add information about meson and link build instructions
by Angel Pons (Code Review) 09 Dec '22

09 Dec '22

Attention is currently required from: Anastasia Klimchuk. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/70176 ) Change subject: README: Add information about meson and link build instructions ...................................................................... Patch Set 1: Code-Review+1 (3 comments) File README: https://review.coreboot.org/c/flashrom/+/70176/comment/dd049a5a_abf895d5 PS1, Line 49: Flashrom nit: `flashrom` should not capitalised. if the idea of starting a sentence without capitalising the first letter of the first word is too painful, we can easily work around this problem with a slight rephrasing: > It is possible to build flashrom using make or meson. https://review.coreboot.org/c/flashrom/+/70176/comment/5159b67d_82b2cea9 PS1, Line 51: although not exactly : all of them. Full meson support is on the roadmap in the nearest future. Hmmm, if meson support isn't full-featured yet, should we explicitly suggest using make whenever possible, at least for now? We've had issues with distro packagers building flashrom v1.2 with meson: there were some bugs that resulted in the version string remaining empty (so logs wouldn't contain the flashrom version anymore) and the `-o` option not working (because of standalone mode, see CB:42230 for the fix). We're aware of the OBSWG (One Build System Working Group) and the long-term plan to drop make in favor of meson. We would then update the README accordingly. https://review.coreboot.org/c/flashrom/+/70176/comment/3cb71a13_b2f0c58f PS1, Line 54: /Documentation/building.md Should we convert the README into a README.md and turn this into a link? Doesn't have to be done in this patch. -- To view, visit https://review.coreboot.org/c/flashrom/+/70176 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I2a27d8f2ba42e18be2485ae95bec1b4c874bb4f7 Gerrit-Change-Number: 70176 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Fri, 09 Dec 2022 13:35:18 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: layout: Check return values for strdup in register_include_arg
by Felix Singer (Code Review) 09 Dec '22

09 Dec '22

Attention is currently required from: Anastasia Klimchuk. Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/70006 ) Change subject: layout: Check return values for strdup in register_include_arg ...................................................................... Patch Set 8: (2 comments) File layout.c: https://review.coreboot.org/c/flashrom/+/70006/comment/ffb71b25_1480c209 PS8, Line 140: msg_gerr("Out of memory"); Missing \n https://review.coreboot.org/c/flashrom/+/70006/comment/f0919299_8e161947 PS8, Line 146: msg_gerr("Out of memory"); Missing \n -- To view, visit https://review.coreboot.org/c/flashrom/+/70006 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I6c22196be6847a8c9704f1de936604a51b4b8a28 Gerrit-Change-Number: 70006 Gerrit-PatchSet: 8 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Fri, 09 Dec 2022 12:45:36 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: layout: Check return values for strdup in register_include_arg
by Angel Pons (Code Review) 09 Dec '22

09 Dec '22

Angel Pons has submitted this change. ( https://review.coreboot.org/c/flashrom/+/70006 ) Change subject: layout: Check return values for strdup in register_include_arg ...................................................................... layout: Check return values for strdup in register_include_arg strdup return values should be checked for NULL to catch the potential error case of out of memory. This patch re-writes ternary conditionals so that strdup return values could be checked for all branches fof execution. Follow up on commit 45d50a101e8073191e6d88143990ed91d3bfe815 Ticket: https://ticket.coreboot.org/issues/372 Change-Id: I6c22196be6847a8c9704f1de936604a51b4b8a28 Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/70006 Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: David Hendricks <david.hendricks(a)gmail.com> Reviewed-by: Angel Pons <th3fanbus(a)gmail.com> --- M layout.c 1 file changed, 43 insertions(+), 3 deletions(-) Approvals: build bot (Jenkins): Verified David Hendricks: Looks good to me, approved Angel Pons: Looks good to me, approved Edward O'Callaghan: Looks good to me, approved diff --git a/layout.c b/layout.c index 2d18f83..9cd3e01 100644 --- a/layout.c +++ b/layout.c @@ -120,7 +120,7 @@ struct layout_include_args *tmp; char *colon; char *name; - char *file; + char *file = NULL; /* file is optional, so defaults to NULL */ if (arg == NULL) { msg_gerr("<NULL> is a bad region name.\n"); @@ -133,8 +133,22 @@ msg_gerr("Missing filename parameter in %s\n", arg); return 1; } - name = colon ? strndup(arg, colon - arg) : strdup(arg); - file = colon ? strdup(colon + 1) : NULL; + + if (colon) { + name = strndup(arg, colon - arg); + if (!name) { + msg_gerr("Out of memory"); + goto error; + } + + file = strdup(colon + 1); + if (!file) { + msg_gerr("Out of memory"); + goto error; + } + } else { + name = strdup(arg); + } for (tmp = *args; tmp; tmp = tmp->next) { if (!strcmp(tmp->name, name)) { -- To view, visit https://review.coreboot.org/c/flashrom/+/70006 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I6c22196be6847a8c9704f1de936604a51b4b8a28 Gerrit-Change-Number: 70006 Gerrit-PatchSet: 8 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

[S] Change in flashrom[master]: layout: Check return values for strdup in register_include_arg
by Angel Pons (Code Review) 09 Dec '22

09 Dec '22

Attention is currently required from: Anastasia Klimchuk. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/70006 ) Change subject: layout: Check return values for strdup in register_include_arg ...................................................................... Patch Set 7: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/70006 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I6c22196be6847a8c9704f1de936604a51b4b8a28 Gerrit-Change-Number: 70006 Gerrit-PatchSet: 7 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Fri, 09 Dec 2022 12:30:29 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: layout: Extract parsing include args into a separate function
by Angel Pons (Code Review) 09 Dec '22

09 Dec '22

Attention is currently required from: Anastasia Klimchuk. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/70539 ) Change subject: layout: Extract parsing include args into a separate function ...................................................................... Patch Set 1: Code-Review+2 (1 comment) File layout.c: https://review.coreboot.org/c/flashrom/+/70539/comment/3b7a1fdf_8563bf4f PS1, Line 117: bool Hmmm, returning an error code would've involved changing less things, but it's not a big deal. We don't seem to have any rules to prefer returning error codes (success is 0) or boolean success/failure (success is not-0, aka true), but the former style seems more common in flashrom. No changes needed, just sharing our thoughts. -- To view, visit https://review.coreboot.org/c/flashrom/+/70539 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iba2971846938fe95412f0a69ff3c069ee2d049b6 Gerrit-Change-Number: 70539 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(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-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Fri, 09 Dec 2022 12:30:10 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: layout: Extract parsing include args into a separate function
by Edward O'Callaghan (Code Review) 09 Dec '22

09 Dec '22

Attention is currently required from: Angel Pons, Anastasia Klimchuk. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/70539 ) Change subject: layout: Extract parsing include args into a separate function ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/70539 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iba2971846938fe95412f0a69ff3c069ee2d049b6 Gerrit-Change-Number: 70539 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(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-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Fri, 09 Dec 2022 12:03:40 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: layout: Check return values for strdup in register_include_arg
by Anastasia Klimchuk (Code Review) 09 Dec '22

09 Dec '22

Attention is currently required from: Angel Pons. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/70006 ) Change subject: layout: Check return values for strdup in register_include_arg ...................................................................... Patch Set 7: (2 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/70006/comment/6c8c0790_e9df4daa PS5, Line 9: strdup return values should be checked for NULL to catch the : potential error case of out of memory. > It looks much better. […] I have made something! CB:70539 Thank you for your help! File layout.c: https://review.coreboot.org/c/flashrom/+/70006/comment/3e144692_4382159a PS7, Line 123: char *file = NULL; /* file is optional, so defaults to NULL */ > (consider this comment to be a non-blocking nit since the code is fine as you've written it and Edwa […] Oh thank you for sharing ideas! I did next patch (on the top of this one) to implement Edward's suggestion. But maybe your idea can be one more next step in refactoring? If I do it, I would do one more separate patch. -- To view, visit https://review.coreboot.org/c/flashrom/+/70006 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I6c22196be6847a8c9704f1de936604a51b4b8a28 Gerrit-Change-Number: 70006 Gerrit-PatchSet: 7 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Fri, 09 Dec 2022 06:48:08 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: David Hendricks <david.hendricks(a)gmail.com> Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: layout: Extract parsing include args into a separate function
by Anastasia Klimchuk (Code Review) 09 Dec '22

09 Dec '22

Anastasia Klimchuk has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/70539 ) Change subject: layout: Extract parsing include args into a separate function ...................................................................... layout: Extract parsing include args into a separate function Change-Id: Iba2971846938fe95412f0a69ff3c069ee2d049b6 Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> --- M layout.c 1 file changed, 41 insertions(+), 12 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/39/70539/1 diff --git a/layout.c b/layout.c index 9cd3e01..350e49e 100644 --- a/layout.c +++ b/layout.c @@ -114,42 +114,61 @@ } #endif -/* register an include argument (-i) for later processing */ -int register_include_arg(struct layout_include_args **args, const char *arg) +static bool parse_include_args(const char *arg, char **name, char **file) { - struct layout_include_args *tmp; char *colon; - char *name; - char *file = NULL; /* file is optional, so defaults to NULL */ + char *tmp_name; + char *tmp_file = NULL; /* file is optional, so defaults to NULL */ if (arg == NULL) { msg_gerr("<NULL> is a bad region name.\n"); - return 1; + return false; } /* -i <image>[:<file>] */ colon = strchr(arg, ':'); if (colon && !colon[1]) { msg_gerr("Missing filename parameter in %s\n", arg); - return 1; + return false; } if (colon) { - name = strndup(arg, colon - arg); - if (!name) { + tmp_name = strndup(arg, colon - arg); + if (!tmp_name) { msg_gerr("Out of memory"); goto error; } - file = strdup(colon + 1); - if (!file) { + tmp_file = strdup(colon + 1); + if (!tmp_file) { msg_gerr("Out of memory"); goto error; } } else { - name = strdup(arg); + tmp_name = strdup(arg); } + *name = tmp_name; + *file = tmp_file; + + return true; + +error: + free(tmp_name); + free(tmp_file); + return false; +} + +/* register an include argument (-i) for later processing */ +int register_include_arg(struct layout_include_args **args, const char *arg) +{ + struct layout_include_args *tmp; + char *name; + char *file; + + if (!parse_include_args(arg, &name, &file)) + return 1; + for (tmp = *args; tmp; tmp = tmp->next) { if (!strcmp(tmp->name, name)) { msg_gerr("Duplicate region name: \"%s\".\n", name); -- To view, visit https://review.coreboot.org/c/flashrom/+/70539 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iba2971846938fe95412f0a69ff3c069ee2d049b6 Gerrit-Change-Number: 70539 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newchange

1 0

[M] Change in flashrom[master]: flashrom: Check for flash access restricitons in read_flash()
by Nikolai Artemiev (Code Review) 09 Dec '22

09 Dec '22

Attention is currently required from: Edward O'Callaghan, Nikolai Artemiev. Hello build bot (Jenkins), Edward O'Callaghan, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/70514 to look at the new patch set (#3). Change subject: flashrom: Check for flash access restricitons in read_flash() ...................................................................... flashrom: Check for flash access restricitons in read_flash() Skip read-protected regions if FLASHROM_FLAG_SKIP_UNREADABLE_REGIONS is true. If the flag is false, read operations that include an read-protected region will return an error. BUG=b:260440773 BRANCH=none TEST=flashrom -r on dedede (JSL) Change-Id: I22c795d7d08ef8bf773733d9952967b2fa2ef299 CoAuthored-by: Edward O'Callaghan <quasisec(a)google.com> Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> Signed-off-by: Nikolai Artemiev <nartemiev(a)google.com> --- M flashrom.c 1 file changed, 87 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/14/70514/3 -- To view, visit https://review.coreboot.org/c/flashrom/+/70514 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I22c795d7d08ef8bf773733d9952967b2fa2ef299 Gerrit-Change-Number: 70514 Gerrit-PatchSet: 3 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-MessageType: newpatchset

1 0

[M] Change in flashrom[master]: libflashrom: Add flags to skip unreadable and unwritable regions
by Edward O'Callaghan (Code Review) 09 Dec '22

09 Dec '22

Attention is currently required from: Angel Pons, Nikolai Artemiev. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/70128 ) Change subject: libflashrom: Add flags to skip unreadable and unwritable regions ...................................................................... Patch Set 22: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/70128 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I9b96fb04b863625d2c9f9a00b97c35b3ddb0871b Gerrit-Change-Number: 70128 Gerrit-PatchSet: 22 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Fri, 09 Dec 2022 05:03:14 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit December 2022