Change in ...flashrom[master]: cli: Add error on missing IFD

[Patrick Rudolph (Code Review)]

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 
---
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) {

[Nico Huber (Code Review)]

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.

[Patrick Rudolph (Code Review)]

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 
---
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

[Nico Huber (Code Review)]

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...

[Patrick Rudolph (Code Review)]

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 
---
M libflashrom.c
1 file changed, 3 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/45/33245/4

[Nico Huber (Code Review)]

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

[Patrick Rudolph (Code Review)]

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 
---
M libflashrom.c
1 file changed, 3 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/45/33245/5

[Patrick Rudolph (Code Review)]

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

[Nico Huber (Code Review)]

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

[Nico Huber (Code Review)]

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 
Reviewed-on: https://review.coreboot.org/c/flashrom/+/33245
Tested-by: build bot (Jenkins) 
Reviewed-by: Nico Huber 
---
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;
    	}