[flashrom] [PATCH] Postpone layout file reading

[Stefan Tauner]

On Wed, 11 Jan 2012 03:13:48 +0100
Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> wrote:

> Am 05.01.2012 03:26 schrieb Stefan Tauner:
> > On Mon, 02 Jan 2012 04:06:39 +0100
> > Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> wrote:
> >
> >> +			if (layoutfile) {
> >> +				fprintf(stderr, "Error: --layout specified "
> >> +					"more than once. Aborting.\n");
> > supporting more than one layout file might become handy for some. for
> > example when using the same hardware platform with multiple firmwares
> > one might have a common file which specifies for example the boot block
> > and multiple other files for the different firmwares respectively.
> > far-fetched? probably. hard to implement? not with an easy and ready to
> > use data structure like an innovative linked list! ;)
> >
> > i am not suggesting adding this now. it just sprang to my mind when
> > thinking about this.
> 
> The usage of multiple layout files in one flashrom invocation is a
> usability nightmare. If there are any conflicting region names or any
> conflicting region definitions, which one has precedence?

with all due respect, this is bullshit :)
here is why:
currently there is no check for multiple equally named regions in a
single file either! is precedence documented anywhere? i guess not. :)
the parser in read_romlayout() just does not care. precedence is
defined by the search algorithm in find_romentry() afaics.

if we check in the parser if an equally named region already exists and
bail out in that case, i dont see a problem at all. this is needed
anyway due to the "inconsistency" mentioned above.

side note: layout file parsing should be postponed after argument
processing too. if one specifies -l <file> before -V+ there are no
debug messages from the layout file parser....

btw: what is a "region definition"? do you mean overlapping address
ranges? this is not handled yet either iirc. it does not make sense
either. one might one to include existing PR* regions, FREG regions and
maybe even other things that overlap multiple times... as long as there
are no images/data sources associated with overlapping and *selected*
regions, there is no need for precedence. this would have to be dealt
with in the "layout: Add -i <image>[:<file>] support" patch.

> And we will
> get mails with complaints like "why doesn't specifying multiple layout
> files work for me".

if we ever meet, ill buy you a beer for any such (legit) mail. :)

> Nack from me. If a user does not have the layout file he/she wants,
> he/she can edit an existing one with a text editor and/or write a new
> one. Users who don't understand layout files should be protected from
> shooting themselves with random layout file combinations.

users that dont understand layout files and using multiple ones anyway?
i dont want to save such users TBH, else i would have to remove all
flashrom options other than -R (see today's emails, why even -V can
apparently lead to a bricked system for some users...).
flashrom is a mighty tool, having a safety net or two is very important
and i dont want to remove any, but bringing this argument up here is
not fair imho.

> > side note:
> > we do not (i.e. i did not) check for duplicate -i arguments:
> > Looking for region "pr0"... found.
> > Looking for region "pr0"... found.
> > Using regions: "pr0", "pr0".
> >
> > not a big issue i hope (i did not check it but presume it to be cosmetic
> > only).  
> 
> Argh. This is not really a bug (it can't cause problems AFAICS), but for
> UI consistency reasons we should indeed check for duplicate -i arguments
> and abort flashrom if we find some.

added to my todo list... not on top though, so if someone beats me to
it, i am happy to ack/review too.
-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner