[PATCH] Postpone layout file reading

[Carl-Daniel Hailfinger]

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");
    			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();
+
    /* Does a chip with the requested name exist in the flashchips array? */
    if (chip_to_probe) {
    	for (flash = flashchips; flash && flash->name; flash++)

[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.

[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

[Stefan Tauner]

On Wed, 11 Jan 2012 03:13:48 +0100
Carl-Daniel Hailfinger  wrote:
> Am 05.01.2012 03:26 schrieb Stefan Tauner:
> > On Mon, 02 Jan 2012 04:06:39 +0100
> > Carl-Daniel Hailfinger  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.

[Stefan Tauner]

On Thu, 12 Jan 2012 01:38:23 +0100
Stefan Tauner  wrote:
> 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....

this nonsense is even funnier to read, if you know that i reimplemented
the committed patch i acked in this very thread. i think ill better call
it a day now. :)

[PATCH] Check for duplicate -i arguments.

[Stefan Tauner]

And a tiny cleanup.
---
./flashrom -p dummy:emulate=M25P10.RES  -c M25P10 -i blub -i blub
flashrom v0.9.5-r1505 on Linux 2.6.35-32-generic (x86_64), built with libpci 3.1.7, GCC 4.4.5, little endian
flashrom is free software, get the source code at http://www.flashrom.org

Duplicate region name: "blub".
Please run "flashrom --help" for usage info.

Signed-off-by: Stefan Tauner 
---
 layout.c |   30 +++++++++++++++++++++++-------
 1 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/layout.c b/layout.c
index 90d3cce..293ae8b 100644
--- a/layout.c
+++ b/layout.c
@@ -201,6 +201,17 @@ int read_romlayout(char *name)
 }
 #endif

+/* returns the index of the entry (or a negative value if it is not found) */
+int find_include_arg(const char *const name)
+{
+	unsigned int i;
+	for (i = 0; i < num_include_args; i++) {
+		if (!strcmp(include_args[i], name))
+			return i;
+	}
+	return -1;
+}
+
 /* register an include argument (-i) for later processing */
 int register_include_arg(char *name)
 {
@@ -214,6 +225,11 @@ int register_include_arg(char *name)
    	return 1;
    }

+	if (find_include_arg(name) != -1) {
+		msg_gerr("Duplicate region name: "%s".\n", name);
+		return 1;
+	}
+
    include_args[num_include_args] = name;
    num_include_args++;
    return 0;
@@ -250,15 +266,15 @@ int process_include_args(void)
    if (num_include_args == 0)
    	return 0;

+	if (!romimages) {
+		msg_gerr("Region requested (with -i "%s"), "
+			 "but no layout data is available.\n",
+			 include_args[0]);
+		return 1;
+	}
+
    for (i = 0; i < num_include_args; i++) {
    	/* User has specified an area, but no layout file is loaded. */
-		if (!romimages) {
-			msg_gerr("Region requested (with -i "%s"), "
-				 "but no layout data is available.\n",
-				 include_args[i]);
-			return 1;
-		}
-
    	if (find_romentry(include_args[i]) < 0) {
    		msg_gerr("Invalid region specified: "%s"\n",
    			 include_args[i]);

[PATCH] [NOT FOR MERGE] use getopt_long's flag field to distinguish short and long options in error messages

[Stefan Tauner]

and fail miserably if error messages for an option depends on another one :)

Signed-off-by: Stefan Tauner 
---
 cli_classic.c |   50 ++++++++++++++++++++++++++++++--------------------
 1 files changed, 30 insertions(+), 20 deletions(-)

diff --git a/cli_classic.c b/cli_classic.c
index 7ce74e5..9693c43 100644
--- a/cli_classic.c
+++ b/cli_classic.c
@@ -178,22 +178,23 @@ int main(int argc, char *argv[])
    int ret = 0;

static const char optstring[] = "r:Rw:v:nVEfc:m:l:i:p:Lzh";
-	static const struct option long_options[] = {
-		{"read",		1, NULL, 'r'},
-		{"write",		1, NULL, 'w'},
-		{"erase",		0, NULL, 'E'},
-		{"verify",		1, NULL, 'v'},
-		{"noverify",		0, NULL, 'n'},
-		{"chip",		1, NULL, 'c'},
-		{"verbose",		0, NULL, 'V'},
-		{"force",		0, NULL, 'f'},
-		{"layout",		1, NULL, 'l'},
-		{"image",		1, NULL, 'i'},
-		{"list-supported",	0, NULL, 'L'},
-		{"list-supported-wiki",	0, NULL, 'z'},
-		{"programmer",		1, NULL, 'p'},
-		{"help",		0, NULL, 'h'},
-		{"version",		0, NULL, 'R'},
+	static int blub = 0;
+	static struct option long_options[] = {
+		{"read",		1, &blub, 'r'},
+		{"write",		1, &blub, 'w'},
+		{"erase",		0, &blub, 'E'},
+		{"verify",		1, &blub, 'v'},
+		{"noverify",		0, &blub, 'n'},
+		{"chip",		1, &blub, 'c'},
+		{"verbose",		0, &blub, 'V'},
+		{"force",		0, &blub, 'f'},
+		{"layout",		1, &blub, 'l'},
+		{"image",		1, &blub, 'i'},
+		{"list-supported",	0, &blub, 'L'},
+		{"list-supported-wiki",	0, &blub, 'z'},
+		{"programmer",		1, &blub, 'p'},
+		{"help",		0, &blub, 'h'},
+		{"version",		0, &blub, 'R'},
    	{NULL,			0, NULL, 0},
    };

@@ -213,6 +214,13 @@ int main(int argc, char *argv[])
     */
    while ((opt = getopt_long(argc, argv, optstring,
    			  long_options, &option_index)) != EOF) {
+		int long_opt = 0;
+		if (opt == 0) {
+			long_opt = 1;
+			opt = long_options[option_index].val;
+		}
+		printf("long_opt = %d\n", long_opt);
+			
    	switch (opt) {
    	case 'r':
    		if (++operation_specified > 1) {
@@ -240,8 +248,9 @@ int main(int argc, char *argv[])
    			cli_classic_abort_usage();
    		}
    		if (dont_verify_it) {
-				fprintf(stderr, "--verify and --noverify are"
-					"mutually exclusive. Aborting.\n");
+				fprintf(stderr, "%s and --noverify are "
+					"mutually exclusive. Aborting.\n",
+					long_opt ? "--verify" : "-v");
    			cli_classic_abort_usage();
    		}
    		filename = strdup(optarg);
@@ -249,8 +258,9 @@ int main(int argc, char *argv[])
    		break;
    	case 'n':
    		if (verify_it) {
-				fprintf(stderr, "--verify and --noverify are"
-					"mutually exclusive. Aborting.\n");
+				fprintf(stderr, "--verify and %s are "
+					"mutually exclusive. Aborting.\n",
+					long_opt ? "--noverify" : "-n");
    			cli_classic_abort_usage();
    		}
    		dont_verify_it = 1;