November 2024 - flashrom-gerrit

[S] Change in flashrom[1.3.x]: libflashrom: Fix comparison of layout entries
by Anastasia Klimchuk (Code Review) 21 Nov '24

21 Nov '24

Anastasia Klimchuk has abandoned this change. ( https://review.coreboot.org/c/flashrom/+/72433?usp=email ) Change subject: libflashrom: Fix comparison of layout entries ...................................................................... Abandoned Follow https://ticket.coreboot.org/issues/570 to track the work on main branch -- To view, visit https://review.coreboot.org/c/flashrom/+/72433?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: abandon Gerrit-Project: flashrom Gerrit-Branch: 1.3.x Gerrit-Change-Id: I125d3892d9efc68e8fc19eef559c82d46c3bdc94 Gerrit-Change-Number: 72433 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>

1 0

[S] Change in flashrom[main]: libflashrom: Fix comparison of layout entries
by Anastasia Klimchuk (Code Review) 21 Nov '24

21 Nov '24

Anastasia Klimchuk has abandoned this change. ( https://review.coreboot.org/c/flashrom/+/85201?usp=email ) Change subject: libflashrom: Fix comparison of layout entries ...................................................................... Abandoned Follow https://ticket.coreboot.org/issues/570 to track the work on main branch -- To view, visit https://review.coreboot.org/c/flashrom/+/85201?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: abandon Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I125d3892d9efc68e8fc19eef559c82d46c3bdc94 Gerrit-Change-Number: 85201 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>

1 0

[S] Change in flashrom[main]: libflashrom: Fix comparison of layout entries
by Anastasia Klimchuk (Code Review) 21 Nov '24

21 Nov '24

Anastasia Klimchuk has posted comments on this change by Anastasia Klimchuk. ( https://review.coreboot.org/c/flashrom/+/85201?usp=email ) Change subject: libflashrom: Fix comparison of layout entries ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: > Cherry-picked from 1.3.x, looks like a reasonable fix but was never reviewed. […] `romentry` struct has changed significantly since the original change based on 1.3.x was created, layout comparison function needs to be re-written. Another question is to also check for other instances of the issue. Work will be tracked under https://ticket.coreboot.org/issues/570 -- To view, visit https://review.coreboot.org/c/flashrom/+/85201?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I125d3892d9efc68e8fc19eef559c82d46c3bdc94 Gerrit-Change-Number: 85201 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Thu, 21 Nov 2024 02:15:07 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>

1 0

[M] Change in flashrom[main]: cli_classic.c: Make -r/-w/-v argument optional when using -i
by Anastasia Klimchuk (Code Review) 21 Nov '24

21 Nov '24

Attention is currently required from: Matt DeVillier. Anastasia Klimchuk has posted comments on this change by Matt DeVillier. ( https://review.coreboot.org/c/flashrom/+/85159?usp=email ) Change subject: cli_classic.c: Make -r/-w/-v argument optional when using -i ...................................................................... Patch Set 3: (6 comments) File cli_classic.c: https://review.coreboot.org/c/flashrom/+/85159/comment/a6830f11_b430c68b?us… : PS2, Line 691: if (read_buf_from_include_args(get_layout(flash), newcontents)) > I don't think so - we check if there is a filename following -r/-w/-v, and if so read the file into […] Yeah you are right. I think I wrote this comment in the beginning of reviewing, and then looked through the rest of the change. Resolving this. https://review.coreboot.org/c/flashrom/+/85159/comment/0814c2e2_8c30607c?us… : PS2, Line 727: if (read_buf_from_include_args(get_layout(flash), newcontents)) > same as above Done https://review.coreboot.org/c/flashrom/+/85159/comment/74384811_8da42d86?us… : PS2, Line 1351: then: : * - Do partial read for each -i arg, creating a new file for : * each region where a filename is provided (-i region:filename). : * - Create a ROM-sized file with partially filled content. For each : * -i arg, fill the corresponding offset with content from ROM. > no, -r is the exception here. Yes, now I see it! Your new formatting of the long comment works, it is more readable now! https://review.coreboot.org/c/flashrom/+/85159/comment/74d33814_fca299df?us… : PS2, Line 1357: Rules for writing and verifying: > I went to re-write this section based on your comments, but feel that it is actually correct as-is. […] Readability accomplished. I only have two tiny comments left (for this comment) File cli_classic.c: https://review.coreboot.org/c/flashrom/+/85159/comment/c83ea5bd_cf7eca3b?us… : PS3, Line 1344: There are few trailing spaces (shown in gerrit in bright pink) in this comment https://review.coreboot.org/c/flashrom/+/85159/comment/99152410_a78aae67?us… : PS3, Line 1366: * - If file is specified for -w/-v and no files are specified with -i : * args, then the file is to be used for writing/verifying the entire : * ROM. Maybe this one can be promoted to common rules? It does apply to all of them -r/-w/-v ? -- To view, visit https://review.coreboot.org/c/flashrom/+/85159?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I6eba095d478f1a7bdbc3854627a656f93dd9e452 Gerrit-Change-Number: 85159 Gerrit-PatchSet: 3 Gerrit-Owner: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Comment-Date: Thu, 21 Nov 2024 01:37:41 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Matt DeVillier <matt.devillier(a)gmail.com> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>

1 0

[M] Change in flashrom[main]: cli_classic.c: Make -r/-w/-v argument optional when using -i
by Matt DeVillier (Code Review) 20 Nov '24

20 Nov '24

Attention is currently required from: Anastasia Klimchuk. Matt DeVillier has posted comments on this change by Matt DeVillier. ( https://review.coreboot.org/c/flashrom/+/85159?usp=email ) Change subject: cli_classic.c: Make -r/-w/-v argument optional when using -i ...................................................................... Patch Set 2: (4 comments) File cli_classic.c: https://review.coreboot.org/c/flashrom/+/85159/comment/c7ef2d96_8070bda9?us… : PS2, Line 691: if (read_buf_from_include_args(get_layout(flash), newcontents)) > Should it be `else if`? We can't have both, right? I don't think so - we check if there is a filename following -r/-w/-v, and if so read the file into a buffer. Then we check if there is a filename following the region if -i is used, and if so, read that into the same buffer. So the way it works now, the filename following the region takes precedence, which is consistent with existing behavior and the documentation https://review.coreboot.org/c/flashrom/+/85159/comment/ad117f43_ace3a81c?us… : PS2, Line 727: if (read_buf_from_include_args(get_layout(flash), newcontents)) > Same question, should it be `else if`? same as above https://review.coreboot.org/c/flashrom/+/85159/comment/fc14e833_5823a68d?us… : PS2, Line 1351: then: : * - Do partial read for each -i arg, creating a new file for : * each region where a filename is provided (-i region:filename). : * - Create a ROM-sized file with partially filled content. For each : * -i arg, fill the corresponding offset with content from ROM. > ... then filename specified for `-r` is ignored? […] no, -r is the exception here. https://review.coreboot.org/c/flashrom/+/85159/comment/35f8d3c8_f2c34dca?us… : PS2, Line 1357: Rules for writing and verifying: > I know it wasn't you who wrote these Rules initially, but let's make them more readable (it's hard r […] I went to re-write this section based on your comments, but feel that it is actually correct as-is. I reformatted it slightly for readability. -- To view, visit https://review.coreboot.org/c/flashrom/+/85159?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I6eba095d478f1a7bdbc3854627a656f93dd9e452 Gerrit-Change-Number: 85159 Gerrit-PatchSet: 2 Gerrit-Owner: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Wed, 20 Nov 2024 21:50:42 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>

1 0

[M] Change in flashrom[main]: cli_classic.c: Make -r/-w/-v argument optional when using -i
by Matt DeVillier (Code Review) 20 Nov '24

20 Nov '24

Attention is currently required from: Matt DeVillier. Hello Anastasia Klimchuk, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/85159?usp=email to look at the new patch set (#3). The following approvals got outdated and were removed: Verified+1 by build bot (Jenkins) Change subject: cli_classic.c: Make -r/-w/-v argument optional when using -i ...................................................................... cli_classic.c: Make -r/-w/-v argument optional when using -i Make the filename parameter directly following -r/-w/-v optional, since the -i parameter allows the image to be written to be sourced from multiple files, regions to be read from flash and written to separate image files, and regions to be verified using an image file only containing that region. Since the filename parameter following -r/-w/-v was ignored when a filename was specified following `-i <region>:<filename>`, this patch essentially removes the requirement to provide an unused parameter. Based on https://review.coreboot.org/c/flashrom/+/52362. TEST=run the following commands on a supported board: flashrom -p internal -r /tmp/coreboot.rom flashrom -p internal -r --ifd -i bios:/tmp/coreboot.rom flashrom -p internal -r /tmp/coreboot.rom --ifd -i bios:/tmp/bios.bin flashrom -p internal -w /tmp/coreboot.rom flashrom -p internal -w --ifd -i bios:/tmp/coreboot.rom flashrom -p internal -w /tmp/coreboot.rom --ifd -i bios:/tmp/bios.bin flashrom -p internal -v /tmp/coreboot.rom flashrom -p internal -v --ifd -i bios:/tmp/coreboot.rom flashrom -p internal -v /tmp/coreboot.rom --ifd -i bios:/tmp/bios.bin Change-Id: I6eba095d478f1a7bdbc3854627a656f93dd9e452 Signed-off-by: Matt DeVillier <matt.devillier(a)gmail.com> --- M cli_classic.c M doc/classic_cli_manpage.rst M include/layout.h M layout.c 4 files changed, 123 insertions(+), 23 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/59/85159/3 -- To view, visit https://review.coreboot.org/c/flashrom/+/85159?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: newpatchset Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I6eba095d478f1a7bdbc3854627a656f93dd9e452 Gerrit-Change-Number: 85159 Gerrit-PatchSet: 3 Gerrit-Owner: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Matt DeVillier <matt.devillier(a)gmail.com>

1 0

[S] Change in flashrom[main]: libflashrom: Fix comparison of layout entries
by Anastasia Klimchuk (Code Review) 20 Nov '24

20 Nov '24

Anastasia Klimchuk has posted comments on this change by Anastasia Klimchuk. ( https://review.coreboot.org/c/flashrom/+/85201?usp=email ) Change subject: libflashrom: Fix comparison of layout entries ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: Cherry-picked from 1.3.x, looks like a reasonable fix but was never reviewed. Doesn't build at the moment, needs further work. The code path which is modified is never executed in the current tree (which probably explains why it was not noticed). -- To view, visit https://review.coreboot.org/c/flashrom/+/85201?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I125d3892d9efc68e8fc19eef559c82d46c3bdc94 Gerrit-Change-Number: 85201 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Wed, 20 Nov 2024 02:39:22 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No

1 0

[S] Change in flashrom[main]: libflashrom: Fix comparison of layout entries
by Anastasia Klimchuk (Code Review) 20 Nov '24

20 Nov '24

Anastasia Klimchuk has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/85201?usp=email ) Change subject: libflashrom: Fix comparison of layout entries ...................................................................... libflashrom: Fix comparison of layout entries A `next` pointer was added to `struct romentry` in commit 953c5ad44058 (layout: Use linked list for `struct romentry`). Hence, comparing the whole contents to match entries doesn't work anymore. Solve that by comparing the `start`, `end` and `name` fields individually. The introduced layout_cmp() function could be made a new API, but that's not necessary for the release branch. Change-Id: I125d3892d9efc68e8fc19eef559c82d46c3bdc94 Signed-off-by: Nico Huber <nico.h(a)gmx.de> --- M libflashrom.c 1 file changed, 16 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/01/85201/1 diff --git a/libflashrom.c b/libflashrom.c index dfc86e6..3961e24 100644 --- a/libflashrom.c +++ b/libflashrom.c @@ -306,6 +306,21 @@ } } +static int layout_cmp(const struct romentry *const a, const struct romentry *const b) +{ + int ret; + + ret = (int)a->start - (int)b->start; + if (ret) + return ret; + + ret = (int)a->end - (int)b->end; + if (ret) + return ret; + + return strcmp(a->name, b->name); +} + int flashrom_layout_read_from_ifd(struct flashrom_layout **const layout, struct flashctx *const flashctx, const void *const dump, const size_t len) { @@ -343,7 +358,7 @@ const struct romentry *chip_entry = layout_next(chip_layout, NULL); const struct romentry *dump_entry = layout_next(dump_layout, NULL); - while (chip_entry && dump_entry && !memcmp(chip_entry, dump_entry, sizeof(*chip_entry))) { + while (chip_entry && dump_entry && !layout_cmp(chip_entry, dump_entry)) { chip_entry = layout_next(chip_layout, chip_entry); dump_entry = layout_next(dump_layout, dump_entry); } -- To view, visit https://review.coreboot.org/c/flashrom/+/85201?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: newchange Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I125d3892d9efc68e8fc19eef559c82d46c3bdc94 Gerrit-Change-Number: 85201 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>

1 0

[S] Change in flashrom[main]: cli_classic: Add option to use first detected chip if multiple found
by Anastasia Klimchuk (Code Review) 19 Nov '24

19 Nov '24

Attention is currently required from: David Hendricks, Matt DeVillier. Anastasia Klimchuk has posted comments on this change by Matt DeVillier. ( https://review.coreboot.org/c/flashrom/+/85160?usp=email ) Change subject: cli_classic: Add option to use first detected chip if multiple found ...................................................................... Patch Set 2: (2 comments) Patchset: PS2: > Adding a couple comments which are probably out-of-scope and non-blocking, though you should check w […] Thank you David, this is useful! File cli_classic.c: https://review.coreboot.org/c/flashrom/+/85160/comment/44fb6b68_c05d5b07?us… : PS2, Line 133: " --use-first-chip in cases where multiple chips are detected, use the first one found\n" > here's my problem: if you're flashing internally, you likely have no idea what chip is actually on t […] The context is very useful, thank you. This explains more narrowed-down use case that you want to handle: that was meant for internal only. David made a good point about write-protect feature. Out of several models with the same IDs, some of them won't support WP, some others will. It happened a bunch of times before my eyes that we needed to split "joint" chip definition into two, exactly because newer model supports WP, and people want to use it. And older model, with the same ID, does not support WP. For your use case, are you interested at all in WP feature? If yes, I am not sure it would work with the strategy of choosing first matching model always. If no (you don't care about WP), I just thought about one more thing, a different idea :) Please tell me what you think. If most of chips in this context are new enough to support SFDP, would you consider trying SFDP auto-detection first? Specifically, that's adding `-c "SFDP-capable chip"` to command line, so that instead of going through the full list flashrom only tries to talk to chip as a generic "SFDP-capable chip"? It actually prints the info that is returned in SFDP header (maybe in verbose mode), so maybe some info about chip can be obtained this way. I will check later what exactly is printed. Like you said > But the vast majority of my users (ChromeOS devices owners) have no idea where the flash chip is on their device, how to access it, etc but if the info is printed, then maybe no need to access the chip -- To view, visit https://review.coreboot.org/c/flashrom/+/85160?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I0427b1ef80e4eca16623f0fc9119d79f7dd62551 Gerrit-Change-Number: 85160 Gerrit-PatchSet: 2 Gerrit-Owner: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Attention: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Comment-Date: Tue, 19 Nov 2024 06:28:41 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Matt DeVillier <matt.devillier(a)gmail.com> Comment-In-Reply-To: David Hendricks <david.hendricks(a)gmail.com> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>

1 0

[M] Change in flashrom[main]: cli_classic.c: Make -r/-w/-v argument optional when using -i
by Anastasia Klimchuk (Code Review) 19 Nov '24

19 Nov '24

Attention is currently required from: Matt DeVillier. Anastasia Klimchuk has posted comments on this change by Matt DeVillier. ( https://review.coreboot.org/c/flashrom/+/85159?usp=email ) Change subject: cli_classic.c: Make -r/-w/-v argument optional when using -i ...................................................................... Patch Set 2: (13 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/85159/comment/8ab82ee6_0022e36e?us… : PS1, Line 7: Make -r/-w/-v argument optional > Ack. […] Done Commit Message: https://review.coreboot.org/c/flashrom/+/85159/comment/9f4b04e4_0a86b02c?us… : PS2, Line 16: this patch : essentially removes the requirement to provide an unused parameter Oh I really like this part. And thank you for updating existing examples in the manpage (which were only mentioning write, but now you fixed it). https://review.coreboot.org/c/flashrom/+/85159/comment/9931eee1_5648c8f5?us… : PS2, Line 21: TEST=verify read/write/verify operations successful when specifying : the filename only after the region. As a note, you can just copy-paste the list of commands that you ran to test, might be easier than writing a text about what you have done. But more importantly, if you could ran few tests without `-i` option, just a standard read, write, erase: - correct examples - and also with missing file parameter, this should fail to validate the command line syntax (fail before starting the operation) Running with dummyflasher is totally fine File cli_classic.c: https://review.coreboot.org/c/flashrom/+/85159/comment/d141bd1e_5d43c02f?us… : PS2, Line 691: if (read_buf_from_include_args(get_layout(flash), newcontents)) Should it be `else if`? We can't have both, right? https://review.coreboot.org/c/flashrom/+/85159/comment/a56b13a4_7c4665f5?us… : PS2, Line 727: if (read_buf_from_include_args(get_layout(flash), newcontents)) Same question, should it be `else if`? https://review.coreboot.org/c/flashrom/+/85159/comment/1de64943_4e02cc8a?us… : PS2, Line 1351: then: : * - Do partial read for each -i arg, creating a new file for : * each region where a filename is provided (-i region:filename). : * - Create a ROM-sized file with partially filled content. For each : * -i arg, fill the corresponding offset with content from ROM. ... then filename specified for `-r` is ignored? The rest should be explained in preceding paragraph already, right? https://review.coreboot.org/c/flashrom/+/85159/comment/c0ced021_396befe0?us… : PS2, Line 1357: Rules for writing and verifying: I know it wasn't you who wrote these Rules initially, but let's make them more readable (it's hard right now). Specific points: 1) Rules for reading, writing , verifying should have the same structure (currently not) 2) Make it as a list of all possible combinations: - no filename specified -> error - filename is specified for -r/-w/-v only - filename is specified for -i only -> explain what happens (which is already written here I think, just scattered) - filename is specified for both -r/-w/-v and -i -> -i takes priority, filename in -r/-w/-v is ignored. It is likely that after writing this, you will copy-paste to manpage (in addition to what's there). So it's very useful to write clear text, and then manpage will be a self-service for users! File doc/classic_cli_manpage.rst: https://review.coreboot.org/c/flashrom/+/85159/comment/36e547a3_de1cb468?us… : PS2, Line 57: file Let's do the same formatting as in the first sentence, `**<file>**` https://review.coreboot.org/c/flashrom/+/85159/comment/7f979c7e_767aa92b?us… : PS2, Line 70: file Let's do the same formatting as in the first sentence, `**<file>**` https://review.coreboot.org/c/flashrom/+/85159/comment/77fa166a_f9dccbf1?us… : PS2, Line 100: file Let's do the same formatting as in the first sentence, `**<file>**` https://review.coreboot.org/c/flashrom/+/85159/comment/fc9a6889_efed68d3?us… : PS2, Line 206: <file> same here, same formatting `**<file>**` File include/layout.h: https://review.coreboot.org/c/flashrom/+/85159/comment/ba4ff03c_3f0bc5d5?us… : PS2, Line 70: int register_include_arg(struct layout_include_args **, const char *arg); : int process_include_args(struct flashrom_layout *, const struct layout_include_args *); : int check_include_args_filename(const struct layout_include_args *); : void cleanup_include_args(struct layout_include_args **); If you are in the good mood, maybe you would consider adding few tests for these functions? We have some tests already for functions in layout.c, in tests/layout.c, it would be great to cover some of include args logic too. I thought you might be interested in that processing of include args has tests and protected from breakage ;) Doesn't need to be in this patch, can be a separate patch, any time later. Either way I would be so so happy to get few more tests! Thank you! File layout.c: https://review.coreboot.org/c/flashrom/+/85159/comment/c385b3f4_fee26836?us… : PS2, Line 296: Error: No region file specified Maybe we can give more details, so that there no confusion for the users where the error comes from: > Error: No region file specified for the --include/-i command. -- To view, visit https://review.coreboot.org/c/flashrom/+/85159?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I6eba095d478f1a7bdbc3854627a656f93dd9e452 Gerrit-Change-Number: 85159 Gerrit-PatchSet: 2 Gerrit-Owner: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Comment-Date: Tue, 19 Nov 2024 05:49:12 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Matt DeVillier <matt.devillier(a)gmail.com> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit November 2024