[flashrom] [PATCH] Postpone layout file reading

[Stefan Tauner]

On Mon, 02 Jan 2012 04:06:39 +0100
Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> 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 <c-d.hailfinger.devel.2006 at gmx.net>
> 
> 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 <stefan.tauner at student.tuwien.ac.at>

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

-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner