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

[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)
> @@ -204,6 +204,7 @@
>    };
>
> char *filename = NULL;
> char *layoutfile = NULL;
> char *tempstr = NULL;
> char *pparam = NULL;
>
> @@ -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.

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 :)
> 		cli_classic_abort_usage();
> 	}
> 	layoutfile = strdup(optarg);
> break;
> case 'i':
> 	tempstr = strdup(optarg);
>
> @@ -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).

Acked-by: Stefan Tauner

PS: you may wanna remove the -m short option with this one already.