Re: [flashrom] [PATCH] Postpone layout file reading

[Carl-Daniel Hailfinger]

Am 05.01.2012 03:26 schrieb Stefan Tauner:
> On Mon, 02 Jan 2012 04:06:39 +0100
> Carl-Daniel Hailfinger  wrote:
> > Layout file reading should happen after option parsing like all other
> > file accesses.
> > Guard against multiple --layout parameters.
> >
> > Side note: This fixes an inconsistency which impacts the log file patch.
> >
> > Signed-off-by: Carl-Daniel Hailfinger
> > Index: flashrom-postpone_layoutfile_reading/cli_classic.c
> >
> > --- flashrom-postpone_layoutfile_reading/cli_classic.c	(Revision 1482)
> > +++ flashrom-postpone_layoutfile_reading/cli_classic.c	(Arbeitskopie)
> > @@ -290,9 +291,12 @@
> >    		force = 1;
> >    		break;
> >    	case 'l':
> > 	tempstr = strdup(optarg);
> > 	if (read_romlayout(tempstr))
> > 	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? And we will
get mails with complaints like "why doesn't specifying multiple layout
files work for me".
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.
> i investigated if we can distinguish between short and long options to
> display a correct error message (list the option actually supplied
> instead of a hardcoded string).
> the good news: it is possible actually. the bad news: it is more
> complicated than technically needed, doable, but probably not worth it.
> so of course ill try it in a few :)

You can try this, but please keep in mind that people who are unfamiliar
with flashrom are probably looking at the man page anyway once they get
an error message.
> > @@ -390,9 +394,6 @@
> >    	cli_classic_abort_usage();
> >    }
> >
> > if (process_include_args())
> > cli_classic_abort_usage();
> >
> > /* FIXME: Print the actions flashrom will take. */
> >
> > if (list_supported) {
> >
> > @@ -407,6 +408,11 @@
> >    }
> >  #endif
> > if (layoutfile && read_romlayout(layoutfile))
> > cli_classic_abort_usage();
> > if (process_include_args())
> > cli_classic_abort_usage();
>
> 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.
> Acked-by: Stefan Tauner

Thanks for the review!
Committed in r1484.
> PS: you may wanna remove the -m short option with this one already.

Right, thanks for the reminder.

Regards,
Carl-Daniel