Change in ...flashrom[master]: cli: Add error on missing IFD
Hello Patrick Rudolph, I'd like you to do a code review. Please visit https://review.coreboot.org/c/flashrom/+/33245 to review the following change. Change subject: cli: Add error on missing IFD ...................................................................... cli: Add error on missing IFD When no IFD is present, but the option --ifd is specified, flashrom would just exit without printing a helpful error message. Add error message that IFD could not be read or parsed. Tested on Intel platform without IFD present. Change-Id: Ie1edd7f36f647c52b17799878185d1e69e10d3b0 Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com> --- M cli_classic.c 1 file changed, 10 insertions(+), 4 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/45/33245/1 diff --git a/cli_classic.c b/cli_classic.c index ced08c6..f6bd59b 100644 --- a/cli_classic.c +++ b/cli_classic.c @@ -607,10 +607,16 @@ if (layoutfile) { layout = get_global_layout(); - } else if (ifd && (flashrom_layout_read_from_ifd(&layout, fill_flash, NULL, 0) || - process_include_args(layout))) { - ret = 1; - goto out_shutdown; + } else if (ifd) { + ret = flashrom_layout_read_from_ifd(&layout, fill_flash, NULL, 0); + if (ret) { + msg_gerr("Failed to read or parse the IFD\n"); + ret = 1; + goto out_shutdown; + } else if (process_include_args(layout)) { + ret = 1; + goto out_shutdown; + } } else if (fmap && fmapfile) { struct stat s; if (stat(fmapfile, &s) != 0) { -- To view, visit https://review.coreboot.org/c/flashrom/+/33245 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie1edd7f36f647c52b17799878185d1e69e10d3b0 Gerrit-Change-Number: 33245 Gerrit-PatchSet: 1 Gerrit-Owner: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph@9elements.com> Gerrit-MessageType: newchange
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33245 ) Change subject: cli: Add error on missing IFD ...................................................................... Patch Set 2: (1 comment) https://review.coreboot.org/#/c/33245/2/cli_classic.c File cli_classic.c: https://review.coreboot.org/#/c/33245/2/cli_classic.c@613 PS2, Line 613: read Read failing is already covered in flashrom_layout_read_from_ifd(). Maybe it would be better to add more elaborate messages to the individual error paths therein. -- To view, visit https://review.coreboot.org/c/flashrom/+/33245 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie1edd7f36f647c52b17799878185d1e69e10d3b0 Gerrit-Change-Number: 33245 Gerrit-PatchSet: 2 Gerrit-Owner: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph@9elements.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Nico Huber <nico.h@gmx.de> Gerrit-Comment-Date: Thu, 06 Jun 2019 10:41:14 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
Hello Patrick Rudolph, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/33245 to look at the new patch set (#3). Change subject: cli: Add error on missing IFD ...................................................................... cli: Add error on missing IFD When no IFD is present, but the option --ifd is specified, flashrom would just exit without printing a helpful error message. Add error message that IFD could not be read or parsed. Tested on Intel platform without IFD present. Change-Id: Ie1edd7f36f647c52b17799878185d1e69e10d3b0 Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com> --- M cli_classic.c M libflashrom.c M libflashrom.h 3 files changed, 36 insertions(+), 4 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/45/33245/3 -- To view, visit https://review.coreboot.org/c/flashrom/+/33245 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie1edd7f36f647c52b17799878185d1e69e10d3b0 Gerrit-Change-Number: 33245 Gerrit-PatchSet: 3 Gerrit-Owner: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph@9elements.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Nico Huber <nico.h@gmx.de> Gerrit-MessageType: newpatchset
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33245 ) Change subject: cli: Add error on missing IFD ...................................................................... Patch Set 3: (2 comments) I am generally not opposed to strerror() like functions. But if we start writing those, we should probably unify the lib- flashrom return values, first? Maybe use a single enum and a single function returning error messages. https://review.coreboot.org/#/c/33245/3/cli_classic.c File cli_classic.c: https://review.coreboot.org/#/c/33245/3/cli_classic.c@613 PS3, Line 613: msg_gerr("Failed to read layout: %s\n", flashrom_layout_read_from_ifd_error_msg(ret)); With flashrom_layout_read_from_ifd() unaltered, it would now print the following on a read failure: Reading ich descriptor... Read operation failed! FAILED! Failed to read layout: descriptor on flash couldn't be read That seems a bit overkill. https://review.coreboot.org/#/c/33245/3/libflashrom.c File libflashrom.c: https://review.coreboot.org/#/c/33245/3/libflashrom.c@392 PS3, Line 392: const char *flashrom_layout_read_from_ifd_error_msg(const int err) It's generally a nice idea, but about the opposite how everything else in flashrom does it: Directly send the error message to the log. I know the flashrom style is a bit hard to get into. But mixing different error-reporting mechanism might make it worse... -- To view, visit https://review.coreboot.org/c/flashrom/+/33245 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie1edd7f36f647c52b17799878185d1e69e10d3b0 Gerrit-Change-Number: 33245 Gerrit-PatchSet: 3 Gerrit-Owner: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph@9elements.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Nico Huber <nico.h@gmx.de> Gerrit-Comment-Date: Fri, 07 Jun 2019 17:15:34 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
Hello Patrick Rudolph, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/33245 to look at the new patch set (#4). Change subject: cli: Add error on missing IFD ...................................................................... cli: Add error on missing IFD When no IFD is present, but the option --ifd is specified, flashrom would just exit without printing a helpful error message. Add error message that IFD could not be read or parsed. Tested on Intel platform without IFD present. Change-Id: Ie1edd7f36f647c52b17799878185d1e69e10d3b0 Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com> --- M libflashrom.c 1 file changed, 3 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/45/33245/4 -- To view, visit https://review.coreboot.org/c/flashrom/+/33245 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie1edd7f36f647c52b17799878185d1e69e10d3b0 Gerrit-Change-Number: 33245 Gerrit-PatchSet: 4 Gerrit-Owner: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph@9elements.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Nico Huber <nico.h@gmx.de> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-MessageType: newpatchset
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33245 ) Change subject: cli: Add error on missing IFD ...................................................................... Patch Set 4: Code-Review+1 (1 comment) Just one typo... https://review.coreboot.org/#/c/33245/4/libflashrom.c File libflashrom.c: https://review.coreboot.org/#/c/33245/4/libflashrom.c@370 PS4, Line 370: doesn't don't -- To view, visit https://review.coreboot.org/c/flashrom/+/33245 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie1edd7f36f647c52b17799878185d1e69e10d3b0 Gerrit-Change-Number: 33245 Gerrit-PatchSet: 4 Gerrit-Owner: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Nico Huber <nico.h@gmx.de> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph@9elements.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Tue, 11 Jun 2019 21:55:02 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Hello Patrick Rudolph, build bot (Jenkins), Nico Huber, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/33245 to look at the new patch set (#5). Change subject: cli: Add error on missing IFD ...................................................................... cli: Add error on missing IFD When no IFD is present, but the option --ifd is specified, flashrom would just exit without printing a helpful error message. Add error message that IFD could not be read or parsed. Tested on Intel platform without IFD present. Change-Id: Ie1edd7f36f647c52b17799878185d1e69e10d3b0 Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com> --- M libflashrom.c 1 file changed, 3 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/45/33245/5 -- To view, visit https://review.coreboot.org/c/flashrom/+/33245 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie1edd7f36f647c52b17799878185d1e69e10d3b0 Gerrit-Change-Number: 33245 Gerrit-PatchSet: 5 Gerrit-Owner: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Nico Huber <nico.h@gmx.de> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph@9elements.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-MessageType: newpatchset
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33245 ) Change subject: cli: Add error on missing IFD ...................................................................... Patch Set 5: (1 comment) https://review.coreboot.org/#/c/33245/4/libflashrom.c File libflashrom.c: https://review.coreboot.org/#/c/33245/4/libflashrom.c@370 PS4, Line 370: doesn't
don't Done
-- To view, visit https://review.coreboot.org/c/flashrom/+/33245 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie1edd7f36f647c52b17799878185d1e69e10d3b0 Gerrit-Change-Number: 33245 Gerrit-PatchSet: 5 Gerrit-Owner: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Nico Huber <nico.h@gmx.de> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph@9elements.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Fri, 14 Jun 2019 15:33:15 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h@gmx.de> Gerrit-MessageType: comment
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33245 ) Change subject: cli: Add error on missing IFD ...................................................................... Patch Set 5: Code-Review+2 Thanks -- To view, visit https://review.coreboot.org/c/flashrom/+/33245 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie1edd7f36f647c52b17799878185d1e69e10d3b0 Gerrit-Change-Number: 33245 Gerrit-PatchSet: 5 Gerrit-Owner: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Nico Huber <nico.h@gmx.de> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph@9elements.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Fri, 14 Jun 2019 16:00:23 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Nico Huber has submitted this change and it was merged. ( https://review.coreboot.org/c/flashrom/+/33245 ) Change subject: cli: Add error on missing IFD ...................................................................... cli: Add error on missing IFD When no IFD is present, but the option --ifd is specified, flashrom would just exit without printing a helpful error message. Add error message that IFD could not be read or parsed. Tested on Intel platform without IFD present. Change-Id: Ie1edd7f36f647c52b17799878185d1e69e10d3b0 Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com> Reviewed-on: https://review.coreboot.org/c/flashrom/+/33245 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Nico Huber <nico.h@gmx.de> --- M libflashrom.c 1 file changed, 3 insertions(+), 0 deletions(-) Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved diff --git a/libflashrom.c b/libflashrom.c index f90a22c..c5e56ae 100644 --- a/libflashrom.c +++ b/libflashrom.c @@ -353,18 +353,21 @@ msg_cinfo("done.\n"); if (layout_from_ich_descriptors(chip_layout, desc, 0x1000)) { + msg_cerr("Couldn't parse the descriptor!\n"); ret = 3; goto _finalize_ret; } if (dump) { if (layout_from_ich_descriptors(&dump_layout, dump, len)) { + msg_cerr("Couldn't parse the descriptor!\n"); ret = 4; goto _finalize_ret; } if (chip_layout->base.num_entries != dump_layout.base.num_entries || memcmp(chip_layout->entries, dump_layout.entries, sizeof(dump_layout.entries))) { + msg_cerr("Descriptors don't match!\n"); ret = 5; goto _finalize_ret; } -- To view, visit https://review.coreboot.org/c/flashrom/+/33245 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie1edd7f36f647c52b17799878185d1e69e10d3b0 Gerrit-Change-Number: 33245 Gerrit-PatchSet: 6 Gerrit-Owner: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Nico Huber <nico.h@gmx.de> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph@9elements.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-MessageType: merged
participants (2)
-
Nico Huber (Code Review) -
Patrick Rudolph (Code Review)