Change in ...flashrom[master]: layout: Introduce layout_next()

[Nico Huber (Code Review)]

Hello Angel Pons, Arthur Heymans, David Hendricks, Thomas Heijligen,

I'd like you to do a code review. Please visit

https://review.coreboot.org/c/flashrom/+/33542

to review the following change.

Change subject: layout: Introduce layout_next()
......................................................................

layout: Introduce layout_next()

And use it to compare the layouts in flashrom_layout_read_from_ifd()
in depth.

Change-Id: I284958471c61344d29d92c95d88475065a9ca9aa
Signed-off-by: Nico Huber 
---
M layout.c
M layout.h
M libflashrom.c
3 files changed, 20 insertions(+), 7 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/42/33542/1

diff --git a/layout.c b/layout.c
index e49189e..684615c 100644
--- a/layout.c
+++ b/layout.c
@@ -252,7 +252,7 @@
    return lowest;
 }

-const struct romentry *layout_next_included(
+const struct romentry *layout_next(
    	const struct flashrom_layout *const layout, const struct romentry *iterator)
 {
    const struct romentry *const end = layout->entries + layout->num_entries;
@@ -262,14 +262,21 @@
    else
    	iterator = &layout->entries[0];

-	for (; iterator < end; ++iterator) {
-		if (!iterator->included)
-			continue;
+	if (iterator < end)
    	return iterator;
-	}
    return NULL;
 }

+const struct romentry *layout_next_included(
+		const struct flashrom_layout *const layout, const struct romentry *iterator)
+{
+	while ((iterator = layout_next(layout, iterator))) {
+		if (iterator->included)
+			break;
+	}
+	return iterator;
+}
+
 /**
  * @addtogroup flashrom-layout
  * @{
diff --git a/layout.h b/layout.h
index 53a20d6..f5444e2 100644
--- a/layout.h
+++ b/layout.h
@@ -66,5 +66,6 @@
 int process_include_args(struct flashrom_layout *l, const struct layout_include_args *const args);
 const struct romentry *layout_next_included_region(const struct flashrom_layout *, chipoff_t);
 const struct romentry *layout_next_included(const struct flashrom_layout *, const struct romentry *);
+const struct romentry *layout_next(const struct flashrom_layout *, const struct romentry *);

#endif				/* !__LAYOUT_H__ */
diff --git a/libflashrom.c b/libflashrom.c
index af62002..6743d50 100644
--- a/libflashrom.c
+++ b/libflashrom.c
@@ -345,8 +345,13 @@
    		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))) {
+		const struct romentry *chip_entry = layout_next(&chip_layout->base, NULL);
+		const struct romentry *dump_entry = layout_next(&dump_layout.base, NULL);
+		while (chip_entry && dump_entry && !memcmp(chip_entry, dump_entry, sizeof(*chip_entry))) {
+			chip_entry = layout_next(&chip_layout->base, chip_entry);
+			dump_entry = layout_next(&dump_layout.base, dump_entry);
+		}
+		if (chip_entry || dump_entry) {
    		msg_cerr("Descriptors don't match!\n");
    		ret = 5;
    		goto _finalize_ret;

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33542 )

Change subject: layout: Introduce layout_next()
......................................................................

Patch Set 1: Code-Review+1

[Nico Huber (Code Review)]

Hello Angel Pons, Arthur Heymans, David Hendricks, Thomas Heijligen, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/flashrom/+/33542

to look at the new patch set (#2).

Change subject: layout: Introduce layout_next()
......................................................................

layout: Introduce layout_next()

Also a `layout.c` internal version _layout_next(), that allows to modify
layout entries.

Use both where applicable and the code is not dropped in this train,
and also to compare the layouts in flashrom_layout_read_from_ifd() in
depth.

Change-Id: I284958471c61344d29d92c95d88475065a9ca9aa
Signed-off-by: Nico Huber 
---
M layout.c
M layout.h
M libflashrom.c
3 files changed, 46 insertions(+), 28 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/42/33542/2

Change in flashrom[master]: layout: Introduce layout_next()

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33542 )

Change subject: layout: Introduce layout_next()
......................................................................

Patch Set 2: Code-Review+1

(2 comments)

https://review.coreboot.org/c/flashrom/+/33542/2/layout.c 
File layout.c:

https://review.coreboot.org/c/flashrom/+/33542/2/layout.c@274 
PS2, Line 274: 
             : const struct romentry *layout_next_included(
             : 		const struct flashrom_layout *const layout, const struct romentry *iterator)
             : {
             : 	while ((iterator = layout_next(layout, iterator))) {
             : 		if (iterator->included)
             : 			break;
             : 	}
             : 	return iterator;
             : }
Maybe move this above _layout_next to align with the header file

https://review.coreboot.org/c/flashrom/+/33542/2/libflashrom.c 
File libflashrom.c:

https://review.coreboot.org/c/flashrom/+/33542/2/libflashrom.c@351 
PS2, Line 351: 			chip_entry = layout_next(&chip_layout->base, chip_entry);
             : 			dump_entry = layout_next(&dump_layout.base, dump_entry);
Looks weird that one of these uses '->' and the other uses '.'

Change in flashrom[master]: layout: Introduce layout_next()

[Nico Huber (Code Review)]

Attention is currently required from: Edward O'Callaghan, Angel Pons, Daniel Campello, Anastasia Klimchuk, Peter Marheine.
Hello build bot (Jenkins), David Hendricks, Thomas Heijligen, Edward O'Callaghan, Angel Pons, Daniel Campello, Arthur Heymans, Anastasia Klimchuk, Peter Marheine,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/flashrom/+/33542

to look at the new patch set (#3).

Change subject: layout: Introduce layout_next()
......................................................................

layout: Introduce layout_next()

Also, a `layout.c` internal version _layout_next() that allows
to modify layout entries and a shorthand to look up an entry by
name, _layout_entry_by_name().

Use the new functions where applicable and the code is not
dropped later in this train, and also to compare the layouts
in flashrom_layout_read_from_ifd() in depth.

Change-Id: I284958471c61344d29d92c95d88475065a9ca9aa
Signed-off-by: Nico Huber 
---
M layout.c
M layout.h
M libflashrom.c
3 files changed, 88 insertions(+), 66 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/42/33542/3

Change in flashrom[master]: layout: Introduce layout_next()

[Nico Huber (Code Review)]

Attention is currently required from: Edward O'Callaghan, Angel Pons, Daniel Campello, Anastasia Klimchuk, Peter Marheine.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33542 )

Change subject: layout: Introduce layout_next()
......................................................................

Patch Set 3:

(2 comments)

File layout.c:

https://review.coreboot.org/c/flashrom/+/33542/comment/3cfbbb18_834637c4 
PS2, Line 274: 
             : const struct romentry *layout_next_included(
             : 		const struct flashrom_layout *const layout, const struct romentry *iterator)
             : {
             : 	while ((iterator = layout_next(layout, iterator))) {
             : 		if (iterator->included)
             : 			break;
             : 	}
             : 	return iterator;
             : }
> Maybe move this above _layout_next to align with the header file

Done

File libflashrom.c:

https://review.coreboot.org/c/flashrom/+/33542/comment/c234f907_7ce8a329 
PS2, Line 351: 			chip_entry = layout_next(&chip_layout->base, chip_entry);
             : 			dump_entry = layout_next(&dump_layout.base, dump_entry);
> Looks weird that one of these uses '->' and the other uses '. […]

Anything to do about it? I could add more pointers, but I don't think
it would become more readable. It will get unified later in this train
anyway.

Change in flashrom[master]: layout: Introduce layout_next()

[Nico Huber (Code Review)]

Attention is currently required from: Edward O'Callaghan, Angel Pons, Daniel Campello, Anastasia Klimchuk, Peter Marheine.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33542 )

Change subject: layout: Introduce layout_next()
......................................................................

Patch Set 3:

(1 comment)

Patchset:

PS3: 
The current rebase is completely untested. Still, I guess
it's ready for comments :)

There are two main goals, building on each other:
* Contain the details of the layout structure in `layout.c`.
* Only use dynamically allocated (and dynamically growing) layouts.

Change in flashrom[master]: layout: Introduce layout_next()

[Peter Marheine (Code Review)]

Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons, Daniel Campello, Anastasia Klimchuk.
Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33542 )

Change subject: layout: Introduce layout_next()
......................................................................

Patch Set 3:

(1 comment)

File layout.c:

https://review.coreboot.org/c/flashrom/+/33542/comment/44416796_11a2374d 
PS3, Line 44: static struct romentry *_layout_next(
Looks like this is declared without the leading underscore and used both ways.

Change in flashrom[master]: layout: Introduce layout_next()

[Anastasia Klimchuk (Code Review)]

Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons, Daniel Campello.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33542 )

Change subject: layout: Introduce layout_next()
......................................................................

Patch Set 3:

(1 comment)

Commit Message:

https://review.coreboot.org/c/flashrom/+/33542/comment/963d6d73_af679243 
PS3, Line 7: layout: Introduce layout_next()
           : 
           : Also, a `layout.c` internal version _layout_next() 
Maybe I am missing something, but this sounds like there are two functions: layout_next() and _layout_next(). In the code I can see layout_next() in the .h but _layout_next() in .c. How does it work?
I did git grep, couldn't find any existing layout_next().

Change in flashrom[master]: layout: Introduce layout_next()

[Angel Pons (Code Review)]

Attention is currently required from: Nico Huber, Edward O'Callaghan, Daniel Campello.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33542 )

Change subject: layout: Introduce layout_next()
......................................................................

Patch Set 3:

(1 comment)

File layout.c:

https://review.coreboot.org/c/flashrom/+/33542/comment/58d3f062_9e4ae87c 
PS3, Line 390: const struct romentry *layout_next(
Here's layout_next()

Change in flashrom[master]: layout: Introduce layout_next()

[Nico Huber (Code Review)]

Attention is currently required from: Edward O'Callaghan, Daniel Campello, Anastasia Klimchuk, Peter Marheine.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33542 )

Change subject: layout: Introduce layout_next()
......................................................................

Patch Set 3:

(2 comments)

Commit Message:

https://review.coreboot.org/c/flashrom/+/33542/comment/a1c1f9c8_c6ef867b 
PS3, Line 7: layout: Introduce layout_next()
           : 
           : Also, a `layout.c` internal version _layout_next()
> Maybe I am missing something, but this sounds like there are two functions: layout_next() and _layou […]

I added both, the API one uses pointers to const struct romentries
and the internal one pointers to mutable struct romentries.

I could just rename the latter to `mutable_layout_next()`, what
do you think?

File layout.c:

https://review.coreboot.org/c/flashrom/+/33542/comment/2f35d876_5c9d1a66 
PS3, Line 44: static struct romentry *_layout_next(
> Looks like this is declared without the leading underscore and used both ways.

Their signatures differ, see comment thread on the commit message.

Change in flashrom[master]: layout: Introduce layout_next()

[Edward O'Callaghan (Code Review)]

Attention is currently required from: Nico Huber, Daniel Campello, Anastasia Klimchuk, Peter Marheine.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33542 )

Change subject: layout: Introduce layout_next()
......................................................................

Patch Set 4: Code-Review+2

(2 comments)

Commit Message:

https://review.coreboot.org/c/flashrom/+/33542/comment/8e68d3df_38c33b06 
PS3, Line 7: layout: Introduce layout_next()
           : 
           : Also, a `layout.c` internal version _layout_next()
> I added both, the API one uses pointers to const struct romentries […]

The `_` prefix definitely requires extra cognitive load for the causal reader if one is not use to the `_` prefix to mean internal convention. Given that so many functions have such similar names in the file I think the `mutable_layout_next()` alternative you suggested Nico is a better choice to go with.

File layout.c:

https://review.coreboot.org/c/flashrom/+/33542/comment/4da6d379_d9294a3a 
PS3, Line 44: static struct romentry *_layout_next(
> Their signatures differ, see comment thread on the commit message.

Nico is correct, one is const while the other isn't. I think the rename to `mutable_layout_next()` will fix up the confusion.

Change in flashrom[master]: layout: Introduce layout_next()

[Nico Huber (Code Review)]

Attention is currently required from: Nico Huber, Daniel Campello, Anastasia Klimchuk, Peter Marheine.
Hello build bot (Jenkins), David Hendricks, Thomas Heijligen, Edward O'Callaghan, Daniel Campello, Angel Pons, Arthur Heymans, Anastasia Klimchuk, Peter Marheine,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/flashrom/+/33542

to look at the new patch set (#5).

Change subject: layout: Introduce layout_next()
......................................................................

layout: Introduce layout_next()

Also, a `layout.c` internal version _layout_next() that allows
to modify layout entries and a shorthand to look up an entry by
name, _layout_entry_by_name().

Use the new functions where applicable and the code is not
dropped later in this train, and also to compare the layouts
in flashrom_layout_read_from_ifd() in depth.

Change-Id: I284958471c61344d29d92c95d88475065a9ca9aa
Signed-off-by: Nico Huber 
---
M layout.c
M layout.h
M libflashrom.c
3 files changed, 88 insertions(+), 66 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/42/33542/5

Change in flashrom[master]: layout: Introduce layout_next()

[Nico Huber (Code Review)]

Attention is currently required from: Edward O'Callaghan, Daniel Campello, Anastasia Klimchuk, Peter Marheine.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33542 )

Change subject: layout: Introduce layout_next()
......................................................................

Patch Set 5:

(1 comment)

Commit Message:

https://review.coreboot.org/c/flashrom/+/33542/comment/33259c33_bef3e346 
PS3, Line 7: layout: Introduce layout_next()
           : 
           : Also, a `layout.c` internal version _layout_next()
> The `_` prefix definitely requires extra cognitive load for the causal reader if one is not use to t […]

Done

Let me know if this is better :)

Change in flashrom[master]: layout: Introduce layout_next()

[Anastasia Klimchuk (Code Review)]

Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons, Daniel Campello, Peter Marheine.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33542 )

Change subject: layout: Introduce layout_next()
......................................................................

Patch Set 5: Code-Review+1

(2 comments)

Commit Message:

https://review.coreboot.org/c/flashrom/+/33542/comment/e109dfb2_5b63f202 
PS3, Line 7: layout: Introduce layout_next()
           : 
           : Also, a `layout.c` internal version _layout_next()
> Done […]

Yes, thank you, definitely better! Just commit message left (still old one).

File layout.c:

https://review.coreboot.org/c/flashrom/+/33542/comment/99476302_937e3c1d 
PS3, Line 390: const struct romentry *layout_next(
> Here's layout_next()

thanks, I missed it somehow

Change in flashrom[master]: layout: Introduce layout_next()

[Peter Marheine (Code Review)]

Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons, Daniel Campello.
Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33542 )

Change subject: layout: Introduce layout_next()
......................................................................

Patch Set 5: Code-Review+1

Change in flashrom[master]: layout: Introduce layout_next()

[Edward O'Callaghan (Code Review)]

Attention is currently required from: Nico Huber, Angel Pons, Daniel Campello.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33542 )

Change subject: layout: Introduce layout_next()
......................................................................

Patch Set 5: Code-Review+2

Change in flashrom[master]: layout: Introduce layout_next()

[Nico Huber (Code Review)]

Attention is currently required from: Edward O'Callaghan, Angel Pons, Daniel Campello, Anastasia Klimchuk, Peter Marheine.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33542 )

Change subject: layout: Introduce layout_next()
......................................................................

Patch Set 7:

(3 comments)

Commit Message:

https://review.coreboot.org/c/flashrom/+/33542/comment/0939b4b4_83a5a2c7 
PS3, Line 7: layout: Introduce layout_next()
           : 
           : Also, a `layout.c` internal version _layout_next()
> Yes, thank you, definitely better! Just commit message left (still old one).

Done

File layout.c:

https://review.coreboot.org/c/flashrom/+/33542/comment/a45179ff_4bf7efa3 
PS3, Line 44: static struct romentry *_layout_next(
> Nico is correct, one is const while the other isn't. […]

Done?

File libflashrom.c:

https://review.coreboot.org/c/flashrom/+/33542/comment/868a29f0_fd3fe0a7 
PS2, Line 351: 			chip_entry = layout_next(&chip_layout->base, chip_entry);
             : 			dump_entry = layout_next(&dump_layout.base, dump_entry);
> Anything to do about it? I could add more pointers, but I don't think […]

Ping

Change in flashrom[master]: layout: Introduce layout_next()

[Nico Huber (Code Review)]

Attention is currently required from: Edward O'Callaghan, Angel Pons, Daniel Campello, Anastasia Klimchuk, Peter Marheine.
Hello build bot (Jenkins), David Hendricks, Thomas Heijligen, Edward O'Callaghan, Daniel Campello, Angel Pons, Arthur Heymans, Anastasia Klimchuk, Peter Marheine,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/flashrom/+/33542

to look at the new patch set (#7).

Change subject: layout: Introduce layout_next()
......................................................................

layout: Introduce layout_next()

Also, a `layout.c` internal version mutable_layout_next() that
allows to modify layout entries and a shorthand to look up an
entry by name, _layout_entry_by_name().

Use the new functions where applicable and the code is not
dropped later in this train, and also to compare the layouts
in flashrom_layout_read_from_ifd() in depth.

Change-Id: I284958471c61344d29d92c95d88475065a9ca9aa
Signed-off-by: Nico Huber 
---
M layout.c
M layout.h
M libflashrom.c
3 files changed, 88 insertions(+), 66 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/42/33542/7

Change in flashrom[master]: layout: Introduce layout_next()

[Peter Marheine (Code Review)]

Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons, Daniel Campello, Anastasia Klimchuk.
Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33542 )

Change subject: layout: Introduce layout_next()
......................................................................

Patch Set 7:

(1 comment)

File layout.c:

https://review.coreboot.org/c/flashrom/+/33542/comment/f31dc191_8f9b51fd 
PS3, Line 44: static struct romentry *_layout_next(
> Done?

LGTM

Change in flashrom[master]: layout: Introduce layout_next()

[Anastasia Klimchuk (Code Review)]

Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons, Daniel Campello.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33542 )

Change subject: layout: Introduce layout_next()
......................................................................

Patch Set 7: Code-Review+2

(1 comment)

File layout.c:

https://review.coreboot.org/c/flashrom/+/33542/comment/1a285c11_d07b1e15 
PS7, Line 319: /* Validate and - if needed - normalize layout entries. */
             : int normalize_romentries(const struct flashctx *flash)
Sorry I've only noticed now, but how this function is doing normalization? I see it is doing validation (and returns 1 or 0), where is normalization? Am I missing something?

Change in flashrom[master]: layout: Introduce layout_next()

[Anastasia Klimchuk (Code Review)]

Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons, Daniel Campello.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33542 )

Change subject: layout: Introduce layout_next()
......................................................................

Patch Set 7:

(1 comment)

File layout.c:

https://review.coreboot.org/c/flashrom/+/33542/comment/08d90312_f8d45876 
PS7, Line 319: /* Validate and - if needed - normalize layout entries. */
             : int normalize_romentries(const struct flashctx *flash)
> Sorry I've only noticed now, but how this function is doing normalization? I see it is doing validat […]

I found it later in the chain CB:54285

Change in flashrom[master]: layout: Introduce layout_next()

[Angel Pons (Code Review)]

Attention is currently required from: Nico Huber, Edward O'Callaghan, Daniel Campello.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33542 )

Change subject: layout: Introduce layout_next()
......................................................................

Patch Set 8: Code-Review+2

Change in flashrom[master]: layout: Introduce layout_next()

[Angel Pons (Code Review)]

Attention is currently required from: Nico Huber, Edward O'Callaghan, Daniel Campello.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33542 )

Change subject: layout: Introduce layout_next()
......................................................................

Patch Set 8:

(1 comment)

File libflashrom.c:

https://review.coreboot.org/c/flashrom/+/33542/comment/4256bbd6_039dfaf8 
PS2, Line 351: 			chip_entry = layout_next(&chip_layout->base, chip_entry);
             : 			dump_entry = layout_next(&dump_layout.base, dump_entry);
> Ping

Ack

Change in flashrom[master]: layout: Introduce layout_next()

[Nico Huber (Code Review)]

Nico Huber has submitted this change. ( https://review.coreboot.org/c/flashrom/+/33542 )

Change subject: layout: Introduce layout_next()
......................................................................

layout: Introduce layout_next()

Also, a `layout.c` internal version mutable_layout_next() that
allows to modify layout entries and a shorthand to look up an
entry by name, _layout_entry_by_name().

Use the new functions where applicable and the code is not
dropped later in this train, and also to compare the layouts
in flashrom_layout_read_from_ifd() in depth.

Change-Id: I284958471c61344d29d92c95d88475065a9ca9aa
Signed-off-by: Nico Huber 
Reviewed-on: https://review.coreboot.org/c/flashrom/+/33542
Reviewed-by: Angel Pons 
Reviewed-by: Anastasia Klimchuk 
Reviewed-by: Peter Marheine 
Reviewed-by: Edward O'Callaghan 
Tested-by: build bot (Jenkins) 
---
M layout.c
M layout.h
M libflashrom.c
3 files changed, 88 insertions(+), 66 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Angel Pons: Looks good to me, approved
  Edward O'Callaghan: Looks good to me, approved
  Peter Marheine: Looks good to me, but someone else must approve
  Anastasia Klimchuk: Looks good to me, approved

diff --git a/layout.c b/layout.c
index cd6b458..2446e8f 100644
--- a/layout.c
+++ b/layout.c
@@ -41,6 +41,32 @@
    	return &flashctx->fallback_layout.base;
 }

+static struct romentry *mutable_layout_next(
+		const struct flashrom_layout *const layout, struct romentry *iterator)
+{
+	const struct romentry *const end = layout->entries + layout->num_entries;
+
+	if (iterator)
+		++iterator;
+	else
+		iterator = &layout->entries[0];
+
+	if (iterator < end)
+		return iterator;
+	return NULL;
+}
+
+static struct romentry *_layout_entry_by_name(
+		const struct flashrom_layout *const layout, const char *name)
+{
+	struct romentry *entry = NULL;
+	while ((entry = mutable_layout_next(layout, entry))) {
+		if (!strcmp(entry->name, name))
+			return entry;
+	}
+	return NULL;
+}
+
 #ifndef __LIBPAYLOAD__
 int read_romlayout(const char *name)
 {
@@ -157,14 +183,12 @@
 static int include_region(struct flashrom_layout *const l, const char *name,
    		  const char *file)
 {
-	size_t i;
-	for (i = 0; i < l->num_entries; ++i) {
-		if (!strcmp(l->entries[i].name, name)) {
-			l->entries[i].included = true;
-			if (file)
-				l->entries[i].file = strdup(file);
-			return 0;
-		}
+	struct romentry *const entry = _layout_entry_by_name(l, name);
+	if (entry) {
+		entry->included = true;
+		if (file)
+			entry->file = strdup(file);
+		return 0;
    }
    return 1;
 }
@@ -187,13 +211,11 @@
 int get_region_range(struct flashrom_layout *const l, const char *name,
    	     unsigned int *start, unsigned int *len)
 {
-	size_t i;
-	for (i = 0; i < l->num_entries; ++i) {
-		if (!strcmp(l->entries[i].name, name)) {
-			*start = l->entries[i].start;
-			*len = l->entries[i].end - l->entries[i].start + 1;
-			return 0;
-		}
+	const struct romentry *const entry = _layout_entry_by_name(l, name);
+	if (entry) {
+		*start = entry->start;
+		*len = entry->end - entry->start + 1;
+		return 0;
    }
    return 1;
 }
@@ -246,30 +268,26 @@
 /* returns boolean 1 if any regions overlap, 0 otherwise */
 int included_regions_overlap(const struct flashrom_layout *const l)
 {
-	size_t i;
+	const struct romentry *lhs = NULL;
    int overlap_detected = 0;

-	for (i = 0; i < l->num_entries; i++) {
-		size_t j;
-
-		if (!l->entries[i].included)
+	while ((lhs = layout_next(l, lhs))) {
+		if (!lhs->included)
    		continue;

-		for (j = i + 1; j < l->num_entries; j++) {
-			if (!l->entries[j].included)
+		const struct romentry *rhs = lhs;
+		while ((rhs = layout_next(l, rhs))) {
+			if (rhs->included)
    			continue;

-			if (l->entries[i].start > l->entries[j].end)
+			if (lhs->start > rhs->end)
    			continue;

-			if (l->entries[i].end < l->entries[j].start)
+			if (lhs->end < rhs->start)
    			continue;

-			msg_gdbg("Regions %s [0x%08x-0x%08x] and "
-				"%s [0x%08x-0x%08x] overlap\n",
-				l->entries[i].name, l->entries[i].start,
-				l->entries[i].end, l->entries[j].name,
-				l->entries[j].start, l->entries[j].end);
+			msg_gdbg("Regions %s [0x%08x-0x%08x] and %s [0x%08x-0x%08x] overlap\n",
+				 lhs->name, lhs->start, lhs->end, rhs->name, rhs->start, rhs->end);
    		overlap_detected = 1;
    	}
    }
@@ -305,17 +323,17 @@
    chipsize_t total_size = flash->chip->total_size * 1024;
    int ret = 0;

-	unsigned int i;
-	for (i = 0; i < layout->num_entries; i++) {
-		if (layout->entries[i].start >= total_size || layout->entries[i].end >= total_size) {
-			msg_gwarn("Warning: Address range of region "%s" exceeds the current chip's "
-				  "address space.\n", layout->entries[i].name);
-			if (layout->entries[i].included)
+	const struct romentry *entry = NULL;
+	while ((entry = layout_next(layout, entry))) {
+		if (entry->start >= total_size || entry->end >= total_size) {
+			msg_gwarn("Warning: Address range of region "%s" "
+				  "exceeds the current chip's address space.\n", entry->name);
+			if (entry->included)
    			ret = 1;
    	}
-		if (layout->entries[i].start > layout->entries[i].end) {
+		if (entry->start > entry->end) {
    		msg_gerr("Error: Size of the address range of region "%s" is not positive.\n",
-				  layout->entries[i].name);
+				  entry->name);
    		ret = 1;
    	}
    }
@@ -326,17 +344,18 @@
 void prepare_layout_for_extraction(struct flashctx *flash)
 {
    const struct flashrom_layout *const l = get_layout(flash);
-	unsigned int i, j;
+	struct romentry *entry = NULL;
+	unsigned int i;

-	for (i = 0; i < l->num_entries; ++i) {
-		l->entries[i].included = true;
+	while ((entry = mutable_layout_next(l, entry))) {
+		entry->included = true;

-		if (!l->entries[i].file)
-			l->entries[i].file = strdup(l->entries[i].name);
+		if (!entry->file)
+			entry->file = strdup(entry->name);

-		for (j = 0; l->entries[i].file[j]; j++) {
-			if (isspace(l->entries[i].file[j]))
-				l->entries[i].file[j] = '_';
+		for (i = 0; entry->file[i]; ++i) {
+			if (isspace(entry->file[i]))
+				entry->file[i] = '_';
    	}
    }
 }
@@ -344,16 +363,15 @@
 const struct romentry *layout_next_included_region(
    	const struct flashrom_layout *const l, const chipoff_t where)
 {
-	unsigned int i;
-	const struct romentry *lowest = NULL;
+	const struct romentry *entry = NULL, *lowest = NULL;

-	for (i = 0; i < l->num_entries; ++i) {
-		if (!l->entries[i].included)
+	while ((entry = layout_next(l, entry))) {
+		if (!entry->included)
    		continue;
-		if (l->entries[i].end < where)
+		if (entry->end < where)
    		continue;
-		if (!lowest || lowest->start > l->entries[i].start)
-			lowest = &l->entries[i];
+		if (!lowest || lowest->start > entry->start)
+			lowest = entry;
    }

return lowest;
@@ -362,19 +380,17 @@
 const struct romentry *layout_next_included(
    	const struct flashrom_layout *const layout, const struct romentry *iterator)
 {
-	const struct romentry *const end = layout->entries + layout->num_entries;
-
-	if (iterator)
-		++iterator;
-	else
-		iterator = &layout->entries[0];
-
-	for (; iterator < end; ++iterator) {
-		if (!iterator->included)
-			continue;
-		return iterator;
+	while ((iterator = layout_next(layout, iterator))) {
+		if (iterator->included)
+			break;
    }
-	return NULL;
+	return iterator;
+}
+
+const struct romentry *layout_next(
+		const struct flashrom_layout *const layout, const struct romentry *iterator)
+{
+	return mutable_layout_next(layout, (struct romentry *)iterator);
 }

/**
diff --git a/layout.h b/layout.h
index f574097..d60c89e 100644
--- a/layout.h
+++ b/layout.h
@@ -70,6 +70,7 @@
 int process_include_args(struct flashrom_layout *l, const struct layout_include_args *const args);
 const struct romentry *layout_next_included_region(const struct flashrom_layout *, chipoff_t);
 const struct romentry *layout_next_included(const struct flashrom_layout *, const struct romentry *);
+const struct romentry *layout_next(const struct flashrom_layout *, const struct romentry *);
 int included_regions_overlap(const struct flashrom_layout *const flashrom_layout);
 void prepare_layout_for_extraction(struct flashrom_flashctx *flash);

diff --git a/libflashrom.c b/libflashrom.c
index d999efc..c6a598b 100644
--- a/libflashrom.c
+++ b/libflashrom.c
@@ -470,8 +470,13 @@
    		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))) {
+		const struct romentry *chip_entry = layout_next(&chip_layout->base, NULL);
+		const struct romentry *dump_entry = layout_next(&dump_layout.base, NULL);
+		while (chip_entry && dump_entry && !memcmp(chip_entry, dump_entry, sizeof(*chip_entry))) {
+			chip_entry = layout_next(&chip_layout->base, chip_entry);
+			dump_entry = layout_next(&dump_layout.base, dump_entry);
+		}
+		if (chip_entry || dump_entry) {
    		msg_cerr("Descriptors don't match!\n");
    		ret = 5;
    		goto _finalize_ret;

Change in flashrom[master]: layout: Introduce layout_next()

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33542 )

Change subject: layout: Introduce layout_next()
......................................................................

Patch Set 9:

(2 comments)

File layout.c:

https://review.coreboot.org/c/flashrom/+/33542/comment/adf2bba7_bb829641 
PS9, Line 269: 				"%s [0x%08x-0x%08x] overlap\n",
Seems like a useful message, why is it only debug?

https://review.coreboot.org/c/flashrom/+/33542/comment/9fceb2ef_a7f857c7 
PS9, Line 280: 			if (rhs->included)
Naughty typo!

I hope that's the only bug I've introduced in this series /o\