Change in ...flashrom[master]: [RFC] cli_classic: Add `--mode (read|write|verify|erase)` parameter

[Nico Huber (Code Review)]

Nico Huber has uploaded this change for review. (
https://review.coreboot.org/c/flashrom/+/30979


Change subject: [RFC] cli_classic: Add `--mode (read|write|verify|erase)` parameter
......................................................................

[RFC] cli_classic: Add `--mode (read|write|verify|erase)` parameter

In case the user only wants to specify files for specific layout
regions, we need a way to set the operation without specifying a
file. Instead of making the <filename> argument optional, we can
add a new syntax for this particular purpose. This way, we avoid
complex command line parsing and can do more sanity checks and
provide better error messages.

TODO: Update manpage in case this gets accepted.

Change-Id: Idfba11ec9991aac423b07f68f7dc45e7bebbb06b
Signed-off-by: Nico Huber &lt;nico.huber(a)secunet.com&gt;
---
M cli_classic.c
1 file changed, 42 insertions(+), 2 deletions(-)



  git pull ssh://review.coreboot.org:29418/flashrom refs/changes/79/30979/1

diff --git a/cli_classic.c b/cli_classic.c
index 324e145..d09a8d1 100644
--- a/cli_classic.c
+++ b/cli_classic.c
@@ -39,7 +39,8 @@
 	       "-z|"
 #endif
 	       "-p <programmername>[:<parameters>] [-c
<chipname>]\n"
-	       "[-E|(-r|-w|-v) <file>] [(-l <layoutfile>|--ifd) [-i
<imagename>]...] [-n] [-N] [-f]]\n"
+	       "[(-r|-w|-v|-E) <file>|-m (r|w|v|e)] [(-l <layoutfile>|--ifd)
[-i <region>]...]\n"
+	       "[-n] [-N] [-f]]\n"
 	       "[-V[V[V]]] [-o <logfile>]\n\n", name);
 
 	printf(" -h | --help                        print this help text\n"
@@ -47,6 +48,8 @@
 	       " -r | --read <file>                 read flash and save to
<file>\n"
 	       " -w | --write <file>                write <file> to
flash\n"
 	       " -v | --verify <file>               verify flash against
<file>\n"
+	       " -m | --mode <operation>            for use with --include
<region>:<file>,\n"
+	       "                                    specify the mode of operation
(r|w|v|e)\n"
 	       " -E | --erase                       erase flash memory\n"
 	       " -V | --verbose                     more verbose output\n"
 	       " -c | --chip <chipname>             probe only for specified flash
chip\n"
@@ -123,6 +126,7 @@
 		{"write",		1, NULL, 'w'},
 		{"erase",		0, NULL, 'E'},
 		{"verify",		1, NULL, 'v'},
+		{"mode",		1, NULL, 'm'},
 		{"noverify",		0, NULL, 'n'},
 		{"noverify-all",	0, NULL, 'N'},
 		{"chip",		1, NULL, 'c'},
@@ -200,6 +204,25 @@
 			filename = strdup(optarg);
 			verify_it = 1;
 			break;
+		case 'm':
+			if (++operation_specified > 1) {
+				fprintf(stderr, "More than one operation specified. Aborting.\n");
+				cli_classic_abort_usage();
+			}
+			/* check that `optarg` is a prefix of a known operation */
+			if (strncmp(optarg, "read", strlen(optarg)) == 0) {
+				read_it = 1;
+			} else if (strncmp(optarg, "write", strlen(optarg)) == 0) {
+				write_it = 1;
+			} else if (strncmp(optarg, "verify", strlen(optarg)) == 0) {
+				verify_it = 1;
+			} else if (strncmp(optarg, "erase", strlen(optarg)) == 0) {
+				erase_it = 1;
+			} else {
+				fprintf(stderr, "Unknown operation specified: '%s'\n", optarg);
+				cli_classic_abort_usage();
+			}
+			break;
 		case 'n':
 			if (verify_it) {
 				fprintf(stderr, "--verify and --noverify are mutually exclusive.
Aborting.\n");
@@ -404,7 +427,7 @@
 		cli_classic_abort_usage();
 	}
 
-	if ((read_it | write_it | verify_it) && check_filename(filename,
"image")) {
+	if (filename && check_filename(filename, "image")) {
 		cli_classic_abort_usage();
 	}
 	if (layoutfile && check_filename(layoutfile, "layout")) {
@@ -654,6 +677,23 @@
 	flashrom_flag_set(fill_flash, FLASHROM_FLAG_VERIFY_AFTER_WRITE, !dont_verify_it);
 	flashrom_flag_set(fill_flash, FLASHROM_FLAG_VERIFY_WHOLE_CHIP, !dont_verify_all);
 
+	if (!filename && (read_it | write_it | verify_it)) {
+		if (layout->num_entries == 0) {
+			msg_gerr("Error: The specified operation requires a file.\n");
+			ret = 1;
+			goto out_shutdown;
+		}
+		for (i = 0; i < layout->num_entries; i++) {
+			if (!layout->entries[i].file) {
+				msg_gerr("Error: Region \"%s\" requires a file argument.\n",
+					 layout->entries[i].name);
+				ret = 1;
+			}
+			if (ret)
+				goto out_shutdown;
+		}
+	}
+
 	/* FIXME: We should issue an unconditional chip reset here. This can be
 	 * done once we have a .reset function in struct flashchip.
 	 * Give the chip time to settle.

-- 
To view, visit https://review.coreboot.org/c/flashrom/+/30979
To unsubscribe, or for help writing mail filters, visit
https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Idfba11ec9991aac423b07f68f7dc45e7bebbb06b
Gerrit-Change-Number: 30979
Gerrit-PatchSet: 1
Gerrit-Owner: Nico Huber &lt;nico.h(a)gmx.de&gt;
Gerrit-MessageType: newchange

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. (
https://review.coreboot.org/c/flashrom/+/30979 )

Change subject: [RFC] cli_classic: Add `--mode (read|write|verify|erase)` parameter
......................................................................


Patch Set 1: Code-Review+1

(1 comment)

https://review.coreboot.org/#/c/30979/1/cli_classic.c 
File cli_classic.c:

https://review.coreboot.org/#/c/30979/1/cli_classic.c@42 
PS1, Line 42: (-r|-w|-v|-E)
Separate change?



-- 
To view, visit https://review.coreboot.org/c/flashrom/+/30979
To unsubscribe, or for help writing mail filters, visit
https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Idfba11ec9991aac423b07f68f7dc45e7bebbb06b
Gerrit-Change-Number: 30979
Gerrit-PatchSet: 1
Gerrit-Owner: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Thu, 17 Jan 2019 14:12:54 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[Nico Huber (Code Review)]

Hello Paul Menzel, build bot (Jenkins), 

I'd like you to reexamine a change. Please visit

    https://review.coreboot.org/c/flashrom/+/30979

to look at the new patch set (#2).

Change subject: [RFC] cli_classic: Add `--mode (read|write|verify|erase)` parameter
......................................................................

[RFC] cli_classic: Add `--mode (read|write|verify|erase)` parameter

In case the user only wants to specify files for specific layout
regions, we need a way to set the operation without specifying a
file. Instead of making the <filename> argument optional, we can
add a new syntax for this particular purpose. This way, we avoid
complex command line parsing and can do more sanity checks and
provide better error messages.

TODO: Update manpage in case this gets accepted.

Change-Id: Idfba11ec9991aac423b07f68f7dc45e7bebbb06b
Signed-off-by: Nico Huber &lt;nico.huber(a)secunet.com&gt;
---
M cli_classic.c
1 file changed, 43 insertions(+), 3 deletions(-)


  git pull ssh://review.coreboot.org:29418/flashrom refs/changes/79/30979/2
-- 
To view, visit https://review.coreboot.org/c/flashrom/+/30979
To unsubscribe, or for help writing mail filters, visit
https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Idfba11ec9991aac423b07f68f7dc45e7bebbb06b
Gerrit-Change-Number: 30979
Gerrit-PatchSet: 2
Gerrit-Owner: Nico Huber &lt;nico.h(a)gmx.de&gt;
Gerrit-Reviewer: Paul Menzel &lt;paulepanter(a)users.sourceforge.net&gt;
Gerrit-Reviewer: build bot (Jenkins) &lt;no-reply(a)coreboot.org&gt;
Gerrit-MessageType: newpatchset

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. (
https://review.coreboot.org/c/flashrom/+/30979 )

Change subject: [RFC] cli_classic: Add `--mode (read|write|verify|erase)` parameter
......................................................................


Patch Set 2:

(1 comment)

https://review.coreboot.org/#/c/30979/1/cli_classic.c 
File cli_classic.c:

https://review.coreboot.org/#/c/30979/1/cli_classic.c@42 
PS1, Line 42: (-r|-w|-v|-E)
  Separate change?
Yes, thought about it :) will
do if we ever agree on a command-line syntax.



-- 
To view, visit https://review.coreboot.org/c/flashrom/+/30979
To unsubscribe, or for help writing mail filters, visit
https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Idfba11ec9991aac423b07f68f7dc45e7bebbb06b
Gerrit-Change-Number: 30979
Gerrit-PatchSet: 2
Gerrit-Owner: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Thu, 17 Jan 2019 15:11:26 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-MessageType: comment

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. (
https://review.coreboot.org/c/flashrom/+/30979 )

Change subject: [RFC] cli_classic: Add `--mode (read|write|verify|erase)` parameter
......................................................................


Patch Set 2:

This might make it more confusing for the moment, my goal would
be to have something like

    flashrom -p internal --ifd -i bios:cbfs.bin --mode write #1

for individual regions, and

    flashrom -p internal -i all:some.rom --mode write #2

for the whole flash, where

    flashrom -p internal -w some.rom #3

would be a shorthand for #2.


-- 
To view, visit https://review.coreboot.org/c/flashrom/+/30979
To unsubscribe, or for help writing mail filters, visit
https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Idfba11ec9991aac423b07f68f7dc45e7bebbb06b
Gerrit-Change-Number: 30979
Gerrit-PatchSet: 2
Gerrit-Owner: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Thu, 17 Jan 2019 18:36:17 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[David Hendricks (Code Review)]

David Hendricks has posted comments on this change. (
https://review.coreboot.org/c/flashrom/+/30979 )

Change subject: [RFC] cli_classic: Add `--mode (read|write|verify|erase)` parameter
......................................................................


Patch Set 2:

(1 comment)

  Patch Set 2:
 
 This might make it more confusing for the moment, my goal would
 be to have something like
 
     flashrom -p internal --ifd -i bios:cbfs.bin --mode write #1
 
 for individual regions, and
 
     flashrom -p internal -i all:some.rom --mode write #2
 
 for the whole flash, where
 
     flashrom -p internal -w some.rom #3
 
 would be a shorthand for #2.
In any case, this is a step in the right direction by making full-sized filename optional.
I'll be completely onboard if `flashrom -p internal --ifd -i bios:cbfs.bin -w` can be
shorthand for #1 and `flashrom -p internal -i all:some.rom -w` can be shorthand for #2.

Otherwise, we should should deprecate -w/-r/-E/-v to be consistent for the various use
cases.

https://review.coreboot.org/#/c/30979/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/30979/2//COMMIT_MSG@14 
PS2, Line 14: provide better error messages.
I'm not sure what is so complex... Optional arguments are well-supported by
getopt_long() and non-positional filename was how flashrom originally worked.

Some day I'll get around to rebasing my patches to demonstrate this more clearly...



-- 
To view, visit https://review.coreboot.org/c/flashrom/+/30979
To unsubscribe, or for help writing mail filters, visit
https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Idfba11ec9991aac423b07f68f7dc45e7bebbb06b
Gerrit-Change-Number: 30979
Gerrit-PatchSet: 2
Gerrit-Owner: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Thu, 17 Jan 2019 19:41:57 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. (
https://review.coreboot.org/c/flashrom/+/30979 )

Change subject: [RFC] cli_classic: Add `--mode (read|write|verify|erase)` parameter
......................................................................


Patch Set 2:

(1 comment)

  This might
make it more confusing for the moment, my goal would
 be to have something like
 
     flashrom -p internal --ifd -i bios:cbfs.bin --mode write #1
 
 for individual regions, and
 
     flashrom -p internal -i all:some.rom --mode write #2
 
 for the whole flash, where
 
     flashrom -p internal -w some.rom #3
 
 would be a shorthand for #2.
 
 In any case, this is a step in the right direction by making full-sized filename
optional. I'll be completely onboard if `flashrom -p internal --ifd -i bios:cbfs.bin
-w` can be shorthand for #1 and `flashrom -p internal -i all:some.rom -w` can be shorthand
for #2.
Well, I added a short form `-m` and a prefix check, so `-mw`
would work. `-w` does still ask for a file name.

  Otherwise, we should should deprecate -w/-r/-E/-v to
be consistent for the various use cases.
I thought about that, too. But the old syntax is really handy
when it comes to flashing full files :-/

https://review.coreboot.org/#/c/30979/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/30979/2//COMMIT_MSG@14 
PS2, Line 14: provide better error messages.
  I'm not sure what is so complex... […]
The
parsing part is solved, Arthur looked after that in
23022. Not easy to read, but acceptable.

But the optional arguments make it hard to guess what the
user intended, e.g. I think currently it would tell the
user to specify files for each region even if he maybe
just forgot to specify the full flash-chip file. Having
different syntaxes for each case evades this problem.



-- 
To view, visit https://review.coreboot.org/c/flashrom/+/30979
To unsubscribe, or for help writing mail filters, visit
https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Idfba11ec9991aac423b07f68f7dc45e7bebbb06b
Gerrit-Change-Number: 30979
Gerrit-PatchSet: 2
Gerrit-Owner: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Thu, 17 Jan 2019 20:05:00 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-MessageType: comment

Change in flashrom[master]: [RFC] cli_classic: Add `--mode (read|write|verify|erase)` parameter

[Nico Huber (Code Review)]

Attention is currently required from: David Hendricks, Daniel Campello, Angel Pons,
Anastasia Klimchuk, Nikolai Artemiev.
Nico Huber has posted comments on this change. (
https://review.coreboot.org/c/flashrom/+/30979 )

Change subject: [RFC] cli_classic: Add `--mode (read|write|verify|erase)` parameter
......................................................................


Patch Set 2:

(1 comment)

Patchset:

PS2: 
  I believe the main difference between this approach
and CB:52362 is that this -m argument is only ne […]
TBH, I'm not very convinced
of either version. One downside of the
optional argument that I probably didn't repeat lately: Typos in
the command line can become more severe (but not as bad as with
the non-positional argument). For instance, consider this:

  $ flashrom -p prog --ifd -i bios:bios.bin -r

And then you want to add a `-V` but miss the dash, it would still
be accepted:

  $ flashrom -p prog --ifd -i bios:bios.bin -r V

It's not very bad. IIRC, with the non-positional argument one
could do much funnier things.

On the wiki[1], I once came up with this:

  [-i <region>]... (-r|-w|-v [<region>:]<filename>)...

I'm not sure anymore, why I implemented `--mode` instead.
It seems the classic CLI will grow further anyway. If we'd
start with this:

  [-i <region>[:<file>]]... -r|-w|-v [<filename>]

We could still later add a (-r|-w|-v [[<region>:]<filename>])
notation. Then we'd stay Cr compatible but nobody would have
to use the error-prone optional argument. It would also be
easier to have compile-time switches, e.g. to make the file
argument optional. If we'd decide to go this way, I'd prefer
to have only the latter syntax added to the help text / man
page.

What do you think? The idea is rather new to me, I still
haven't made up my mind.

[1] https://flashrom.org/Per_region_file_arguments#More_explicit_alternative



-- 
To view, visit https://review.coreboot.org/c/flashrom/+/30979
To unsubscribe, or for help writing mail filters, visit
https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Idfba11ec9991aac423b07f68f7dc45e7bebbb06b
Gerrit-Change-Number: 30979
Gerrit-PatchSet: 2
Gerrit-Owner: Nico Huber &lt;nico.h(a)gmx.de&gt;
Gerrit-Reviewer: Anastasia Klimchuk &lt;aklm(a)chromium.org&gt;
Gerrit-Reviewer: Angel Pons &lt;th3fanbus(a)gmail.com&gt;
Gerrit-Reviewer: Arthur Heymans &lt;arthur(a)aheymans.xyz&gt;
Gerrit-Reviewer: Carl-Daniel Hailfinger &lt;hailfinger(a)gmail.com&gt;
Gerrit-Reviewer: Daniel Campello &lt;campello(a)chromium.org&gt;
Gerrit-Reviewer: David Hendricks &lt;david.hendricks(a)gmail.com&gt;
Gerrit-Reviewer: Edward O'Callaghan &lt;quasisec(a)chromium.org&gt;
Gerrit-Reviewer: Nico Huber &lt;nico.h(a)gmx.de&gt;
Gerrit-Reviewer: Nikolai Artemiev &lt;nartemiev(a)google.com&gt;
Gerrit-Reviewer: Paul Menzel &lt;paulepanter(a)mailbox.org&gt;
Gerrit-Reviewer: Stefan T &lt;stefan.tauner(a)gmx.at&gt;
Gerrit-Reviewer: build bot (Jenkins) &lt;no-reply(a)coreboot.org&gt;
Gerrit-Attention: David Hendricks &lt;david.hendricks(a)gmail.com&gt;
Gerrit-Attention: Daniel Campello &lt;campello(a)chromium.org&gt;
Gerrit-Attention: Angel Pons &lt;th3fanbus(a)gmail.com&gt;
Gerrit-Attention: Anastasia Klimchuk &lt;aklm(a)chromium.org&gt;
Gerrit-Attention: Nikolai Artemiev &lt;nartemiev(a)google.com&gt;
Gerrit-Comment-Date: Sat, 08 May 2021 11:43:06 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Daniel Campello &lt;campello(a)chromium.org&gt;
Gerrit-MessageType: comment

Change in flashrom[master]: [RFC] cli_classic: Add `--mode (read|write|verify|erase)` parameter

[Daniel Campello (Code Review)]

Attention is currently required from: Nico Huber, David Hendricks, Edward
O'Callaghan.
Daniel Campello has posted comments on this change. (
https://review.coreboot.org/c/flashrom/+/30979 )

Change subject: [RFC] cli_classic: Add `--mode (read|write|verify|erase)` parameter
......................................................................


Patch Set 2:

(1 comment)

Commit Message:

https://review.coreboot.org/c/flashrom/+/30979/comment/b0e6c807_62ace6b7 
PS2, Line 14: provide better error messages.
  I have to say I didn't really like dealing with
the optional argument in the cros tree David however […]
I'm not sure how the
combinatorics are smaller in this case. I believe is the same amount of cases as I
presented on my previous message.

Re error reporting we can make it clear that both options exist: "Missing region
filename for region <region> when no flash_chip file was provided with
<operation>"

I still think that the UX around keeping the same flag for the same operation (regardless
of the input/output artifacts) is much easier to reason about from the user perspective.
Having -r <file> and -m r[ead] options for reading flash chip in my opinion is
confusing. Just to reiterate what David said optional arguments are not outside of
standard as they are supported by getopt.

Said all of that, if the consensus is for this option I am fine with it.



-- 
To view, visit https://review.coreboot.org/c/flashrom/+/30979
To unsubscribe, or for help writing mail filters, visit
https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Idfba11ec9991aac423b07f68f7dc45e7bebbb06b
Gerrit-Change-Number: 30979
Gerrit-PatchSet: 2
Gerrit-Owner: Nico Huber &lt;nico.h(a)gmx.de&gt;
Gerrit-Reviewer: Arthur Heymans &lt;arthur(a)aheymans.xyz&gt;
Gerrit-Reviewer: Carl-Daniel Hailfinger &lt;hailfinger(a)gmail.com&gt;
Gerrit-Reviewer: Daniel Campello &lt;campello(a)chromium.org&gt;
Gerrit-Reviewer: David Hendricks &lt;david.hendricks(a)gmail.com&gt;
Gerrit-Reviewer: Edward O'Callaghan &lt;quasisec(a)chromium.org&gt;
Gerrit-Reviewer: Nico Huber &lt;nico.h(a)gmx.de&gt;
Gerrit-Reviewer: Paul Menzel &lt;paulepanter(a)mailbox.org&gt;
Gerrit-Reviewer: Stefan T &lt;stefan.tauner(a)gmx.at&gt;
Gerrit-Reviewer: build bot (Jenkins) &lt;no-reply(a)coreboot.org&gt;
Gerrit-Attention: Nico Huber &lt;nico.h(a)gmx.de&gt;
Gerrit-Attention: David Hendricks &lt;david.hendricks(a)gmail.com&gt;
Gerrit-Attention: Edward O'Callaghan &lt;quasisec(a)chromium.org&gt;
Gerrit-Comment-Date: Sat, 08 May 2021 01:46:46 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber &lt;nico.h(a)gmx.de&gt;
Comment-In-Reply-To: David Hendricks &lt;david.hendricks(a)gmail.com&gt;
Comment-In-Reply-To: Edward O'Callaghan &lt;quasisec(a)chromium.org&gt;
Gerrit-MessageType: comment

Change in flashrom[master]: [RFC] cli_classic: Add `--mode (read|write|verify|erase)` parameter

[Edward O'Callaghan (Code Review)]

Attention is currently required from: Nico Huber, David Hendricks.
Edward O'Callaghan has posted comments on this change. (
https://review.coreboot.org/c/flashrom/+/30979 )

Change subject: [RFC] cli_classic: Add `--mode (read|write|verify|erase)` parameter
......................................................................


Patch Set 2:

(1 comment)

Commit Message:

https://review.coreboot.org/c/flashrom/+/30979/comment/b0840b24_3d010793 
PS2, Line 14: provide better error messages.
  The parsing part is solved, Arthur looked after that
in […]
I have to say I didn't really like dealing with the optional argument in
the cros tree David however I couldn't think of a better alternative.

I much much prefer Nico's idea of a mode here, the combinatorics of options is much
smaller and therefore the behavior is much easier to reason about.

+1'ed as I just want to make sure Daniel Campello is on board for this?



-- 
To view, visit https://review.coreboot.org/c/flashrom/+/30979
To unsubscribe, or for help writing mail filters, visit
https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Idfba11ec9991aac423b07f68f7dc45e7bebbb06b
Gerrit-Change-Number: 30979
Gerrit-PatchSet: 2
Gerrit-Owner: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Carl-Daniel Hailfinger <hailfinger(a)gmail.com>
Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org>
Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Reviewer: Stefan T <stefan.tauner(a)gmx.at>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Comment-Date: Sat, 08 May 2021 00:28:26 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-MessageType: comment

Change in flashrom[master]: [RFC] cli_classic: Add `--mode (read|write|verify|erase)` parameter

[Daniel Campello (Code Review)]

Attention is currently required from: Nico Huber, David Hendricks, Edward O'Callaghan,
Angel Pons, Anastasia Klimchuk, Nikolai Artemiev.
Daniel Campello has posted comments on this change. (
https://review.coreboot.org/c/flashrom/+/30979 )

Change subject: [RFC] cli_classic: Add `--mode (read|write|verify|erase)` parameter
......................................................................


Patch Set 2:

(1 comment)

Patchset:

PS2: 
  Yeah, I remember that was a concern. […]
I just
thought of something that may be confusing with the latest proposal:

# flashrom -r file # when no -i arg reads the whole chip
# flashrom -i region -r file # reads only region <region> on the ROM size file with
everything else 0
# flashrom -r region:region_file file # is not obvious what file may have since no -i was
provided

I think given the above it is much cleaner to keep the -i region:file interface.



-- 
To view, visit https://review.coreboot.org/c/flashrom/+/30979
To unsubscribe, or for help writing mail filters, visit
https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Idfba11ec9991aac423b07f68f7dc45e7bebbb06b
Gerrit-Change-Number: 30979
Gerrit-PatchSet: 2
Gerrit-Owner: Nico Huber &lt;nico.h(a)gmx.de&gt;
Gerrit-Reviewer: Anastasia Klimchuk &lt;aklm(a)chromium.org&gt;
Gerrit-Reviewer: Angel Pons &lt;th3fanbus(a)gmail.com&gt;
Gerrit-Reviewer: Arthur Heymans &lt;arthur(a)aheymans.xyz&gt;
Gerrit-Reviewer: Carl-Daniel Hailfinger &lt;hailfinger(a)gmail.com&gt;
Gerrit-Reviewer: Daniel Campello &lt;campello(a)chromium.org&gt;
Gerrit-Reviewer: David Hendricks &lt;david.hendricks(a)gmail.com&gt;
Gerrit-Reviewer: Edward O'Callaghan &lt;quasisec(a)chromium.org&gt;
Gerrit-Reviewer: Nico Huber &lt;nico.h(a)gmx.de&gt;
Gerrit-Reviewer: Nikolai Artemiev &lt;nartemiev(a)google.com&gt;
Gerrit-Reviewer: Paul Menzel &lt;paulepanter(a)mailbox.org&gt;
Gerrit-Reviewer: Stefan T &lt;stefan.tauner(a)gmx.at&gt;
Gerrit-Reviewer: build bot (Jenkins) &lt;no-reply(a)coreboot.org&gt;
Gerrit-Attention: Nico Huber &lt;nico.h(a)gmx.de&gt;
Gerrit-Attention: David Hendricks &lt;david.hendricks(a)gmail.com&gt;
Gerrit-Attention: Edward O'Callaghan &lt;quasisec(a)chromium.org&gt;
Gerrit-Attention: Angel Pons &lt;th3fanbus(a)gmail.com&gt;
Gerrit-Attention: Anastasia Klimchuk &lt;aklm(a)chromium.org&gt;
Gerrit-Attention: Nikolai Artemiev &lt;nartemiev(a)google.com&gt;
Gerrit-Comment-Date: Thu, 13 May 2021 01:11:37 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: David Hendricks &lt;david.hendricks(a)gmail.com&gt;
Comment-In-Reply-To: Daniel Campello &lt;campello(a)chromium.org&gt;
Gerrit-MessageType: comment

Change in flashrom[master]: [RFC] cli_classic: Add `--mode (read|write|verify|erase)` parameter

[Daniel Campello (Code Review)]

Attention is currently required from: Nico Huber, Edward O'Callaghan.
Daniel Campello has posted comments on this change. (
https://review.coreboot.org/c/flashrom/+/30979 )

Change subject: [RFC] cli_classic: Add `--mode (read|write|verify|erase)` parameter
......................................................................


Patch Set 2:

(1 comment)

File cli_classic.c:

https://review.coreboot.org/c/flashrom/+/30979/comment/534642d8_056984a5 
PS2, Line 687: layout->entries[i].file
Here you could do check_filename(layout->entries[i].file, "region") for
additional checks (i.e. != '\0')



-- 
To view, visit https://review.coreboot.org/c/flashrom/+/30979
To unsubscribe, or for help writing mail filters, visit
https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Idfba11ec9991aac423b07f68f7dc45e7bebbb06b
Gerrit-Change-Number: 30979
Gerrit-PatchSet: 2
Gerrit-Owner: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Carl-Daniel Hailfinger <hailfinger(a)gmail.com>
Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org>
Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Reviewer: Stefan T <stefan.tauner(a)gmx.at>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Comment-Date: Fri, 07 May 2021 00:44:59 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

Change in flashrom[master]: [RFC] cli_classic: Add `--mode (read|write|verify|erase)` parameter

[Daniel Campello (Code Review)]

Attention is currently required from: Nico Huber, Edward O'Callaghan.
Daniel Campello has posted comments on this change. (
https://review.coreboot.org/c/flashrom/+/30979 )

Change subject: [RFC] cli_classic: Add `--mode (read|write|verify|erase)` parameter
......................................................................


Patch Set 2:

(1 comment)

Patchset:

PS2: 
I believe the main difference between this approach and CB:52362 is that this -m argument
is only needed if no -r/-v/-w <file> is specified but not needed/allowed otherwise.

With this apporach:
- If someone wants to read a region *and* the whole chip:
# flashrom -i region:region-file.bin -r chip-file.bin
- If someone wants to read a region *and not* the whole chip:
# flashrom -i region:region-file.bin -m r

With CB:52362
- If someone wants to read a region *and* the whole chip:
# flashrom -i region:region-file.bin -r chip-file.bin
- If someone wants to read a region *and not* the whole chip:
# flashrom -i region:region-file.bin -r

I *personally* think having -r do the signaling for both types of reads is cleaner given
that -m won't be used for "normal" full chip reads.

As an a side (not that I'm saying this should be a deciding factor for upstream
direction): the second approach should be compatible with chromium usage and will keep
both CLIs almost ("almost" chromium has a couple of additional flags
--do-not-diff and --ignore-lock) aligned as of today.



-- 
To view, visit https://review.coreboot.org/c/flashrom/+/30979
To unsubscribe, or for help writing mail filters, visit
https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Idfba11ec9991aac423b07f68f7dc45e7bebbb06b
Gerrit-Change-Number: 30979
Gerrit-PatchSet: 2
Gerrit-Owner: Nico Huber &lt;nico.h(a)gmx.de&gt;
Gerrit-Reviewer: Arthur Heymans &lt;arthur(a)aheymans.xyz&gt;
Gerrit-Reviewer: Carl-Daniel Hailfinger &lt;hailfinger(a)gmail.com&gt;
Gerrit-Reviewer: Daniel Campello &lt;campello(a)chromium.org&gt;
Gerrit-Reviewer: David Hendricks &lt;david.hendricks(a)gmail.com&gt;
Gerrit-Reviewer: Edward O'Callaghan &lt;quasisec(a)chromium.org&gt;
Gerrit-Reviewer: Nico Huber &lt;nico.h(a)gmx.de&gt;
Gerrit-Reviewer: Paul Menzel &lt;paulepanter(a)mailbox.org&gt;
Gerrit-Reviewer: Stefan T &lt;stefan.tauner(a)gmx.at&gt;
Gerrit-Reviewer: build bot (Jenkins) &lt;no-reply(a)coreboot.org&gt;
Gerrit-Attention: Nico Huber &lt;nico.h(a)gmx.de&gt;
Gerrit-Attention: Edward O'Callaghan &lt;quasisec(a)chromium.org&gt;
Gerrit-Comment-Date: Fri, 07 May 2021 00:35:36 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

Change in flashrom[master]: [RFC] cli_classic: Add `--mode (read|write|verify|erase)` parameter

[Edward O'Callaghan (Code Review)]

Attention is currently required from: Nico Huber.
Edward O'Callaghan has posted comments on this change. (
https://review.coreboot.org/c/flashrom/+/30979 )

Change subject: [RFC] cli_classic: Add `--mode (read|write|verify|erase)` parameter
......................................................................


Patch Set 2: Code-Review+1


-- 
To view, visit https://review.coreboot.org/c/flashrom/+/30979
To unsubscribe, or for help writing mail filters, visit
https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Idfba11ec9991aac423b07f68f7dc45e7bebbb06b
Gerrit-Change-Number: 30979
Gerrit-PatchSet: 2
Gerrit-Owner: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Carl-Daniel Hailfinger <hailfinger(a)gmail.com>
Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org>
Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Reviewer: Stefan T <stefan.tauner(a)gmx.at>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Comment-Date: Sat, 08 May 2021 00:23:17 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

Change in flashrom[master]: [RFC] cli_classic: Add `--mode (read|write|verify|erase)` parameter

[David Hendricks (Code Review)]

Attention is currently required from: Nico Huber, Edward O'Callaghan, Daniel Campello,
Angel Pons, Anastasia Klimchuk, Nikolai Artemiev.
David Hendricks has posted comments on this change. (
https://review.coreboot.org/c/flashrom/+/30979 )

Change subject: [RFC] cli_classic: Add `--mode (read|write|verify|erase)` parameter
......................................................................


Patch Set 2:

(1 comment)

Patchset:

PS2: 
  > - Backwards-compatible with flashrom syntax prior
to CB:23021 […]
Yeah, I remember that was a concern. Does anybody remember exactly
why? IIRC it was considered complicated, yet non-positional files have been supported and
standard in Unix commands since before most of us were born so I have a hard time
understanding that argument.

Using non-positional files as illustrated in
https://flashrom.org/Per_region_file_arguments#Use_cases enables us to treat chip-sized
files and region-sized files with the same, intuitive syntax without added mode flags or
inconsistent use of -i/-r/-w/-v as in other proposals.



-- 
To view, visit https://review.coreboot.org/c/flashrom/+/30979
To unsubscribe, or for help writing mail filters, visit
https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Idfba11ec9991aac423b07f68f7dc45e7bebbb06b
Gerrit-Change-Number: 30979
Gerrit-PatchSet: 2
Gerrit-Owner: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Carl-Daniel Hailfinger <hailfinger(a)gmail.com>
Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org>
Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Reviewer: Stefan T <stefan.tauner(a)gmx.at>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Daniel Campello <campello(a)chromium.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Comment-Date: Tue, 11 May 2021 17:08:01 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: David Hendricks <david.hendricks(a)gmail.com>
Comment-In-Reply-To: Daniel Campello <campello(a)chromium.org>
Gerrit-MessageType: comment

Change in flashrom[master]: [RFC] cli_classic: Add `--mode (read|write|verify|erase)` parameter

[Daniel Campello (Code Review)]

Attention is currently required from: Nico Huber, David Hendricks, Edward O'Callaghan,
Angel Pons, Anastasia Klimchuk, Nikolai Artemiev.
Daniel Campello has posted comments on this change. (
https://review.coreboot.org/c/flashrom/+/30979 )

Change subject: [RFC] cli_classic: Add `--mode (read|write|verify|erase)` parameter
......................................................................


Patch Set 2:

(1 comment)

Patchset:

PS2: 
  - Backwards-compatible with flashrom syntax prior to
CB:23021
For it to be backwards compatible the file argument may appear anywhere is the cmdline:

 # flashrom -r file -V --noverify-all
 # flashrom -r -V --noverify-all file
 # flashrom -r -V file --noverify-all

I believe this was the main complain of the chromium syntax on CB:23022 which I am
addressing in CB:52362 (and crrev.com/c/2823343)



-- 
To view, visit https://review.coreboot.org/c/flashrom/+/30979
To unsubscribe, or for help writing mail filters, visit
https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Idfba11ec9991aac423b07f68f7dc45e7bebbb06b
Gerrit-Change-Number: 30979
Gerrit-PatchSet: 2
Gerrit-Owner: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Carl-Daniel Hailfinger <hailfinger(a)gmail.com>
Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org>
Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Reviewer: Stefan T <stefan.tauner(a)gmx.at>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Comment-Date: Tue, 11 May 2021 16:37:10 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-MessageType: comment

Change in flashrom[master]: [RFC] cli_classic: Add `--mode (read|write|verify|erase)` parameter

[David Hendricks (Code Review)]

Attention is currently required from: Nico Huber, Edward O'Callaghan, Daniel Campello,
Angel Pons, Anastasia Klimchuk, Nikolai Artemiev.
David Hendricks has posted comments on this change. (
https://review.coreboot.org/c/flashrom/+/30979 )

Change subject: [RFC] cli_classic: Add `--mode (read|write|verify|erase)` parameter
......................................................................


Patch Set 2:

(1 comment)

Patchset:

PS2: 
I came up with another option that I think addresses many of the concerns raised here:
https://flashrom.org/Per_region_file_arguments#Alternative_.233

Advantages:
- No positional or optional argument after -r/-w/-v. This simplifies them and makes them
consistent with -E, so this approach *actually* reduces combinatorics.
- -r/-w/-v is specified exactly once.
- No added modes.
- Arguments are handled similarly to standard Unix commands (ls, rm, cat, etc)
- Backwards-compatible with flashrom syntax prior to CB:23021

LMK what you think.



-- 
To view, visit https://review.coreboot.org/c/flashrom/+/30979
To unsubscribe, or for help writing mail filters, visit
https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Idfba11ec9991aac423b07f68f7dc45e7bebbb06b
Gerrit-Change-Number: 30979
Gerrit-PatchSet: 2
Gerrit-Owner: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Carl-Daniel Hailfinger <hailfinger(a)gmail.com>
Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org>
Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Reviewer: Stefan T <stefan.tauner(a)gmx.at>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Daniel Campello <campello(a)chromium.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Comment-Date: Tue, 11 May 2021 16:21:02 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

Change in flashrom[master]: [RFC] cli_classic: Add `--mode (read|write|verify|erase)` parameter

[David Hendricks (Code Review)]

Attention is currently required from: Nico Huber, Edward O'Callaghan, Daniel Campello,
Angel Pons, Anastasia Klimchuk, Nikolai Artemiev.
David Hendricks has posted comments on this change. (
https://review.coreboot.org/c/flashrom/+/30979 )

Change subject: [RFC] cli_classic: Add `--mode (read|write|verify|erase)` parameter
......................................................................


Patch Set 2: Code-Review-1

(3 comments)

Commit Message:

https://review.coreboot.org/c/flashrom/+/30979/comment/6e27cbd3_d06a4e27 
PS2, Line 14: provide better error messages.
  I'm not sure how the combinatorics are smaller in
this case. I believe is the same amount of cases as I presented on my previous message.
 
 Re error reporting we can make it clear that both options exist: "Missing region
filename for region <region> when no flash_chip file was provided with
<operation>"
 
 I still think that the UX around keeping the same flag for the same operation (regardless
of the input/output artifacts) is much easier to reason about from the user perspective.
Having -r <file> and -m r[ead] options for reading flash chip in my opinion is
confusing. Just to reiterate what David said optional arguments are not outside of
standard as they are supported by getopt.
 
 Said all of that, if the consensus is for this option I am fine with it.
I still don't like the proposed syntax, but if others feel really strongly in favor
then I won't block it.

I agree with Daniel's analysis above. I don't buy the argument that adding another
parameter will somehow decrease combinatorics - I think it will add complexity to the code
and make usage even more confusing. Perhaps I'm missing the bigger picture though.

BTW, to illustrate the UX I've added a table to the wiki:
https://flashrom.org/Per_region_file_arguments#Use_cases

Let's fill that out and see if that helps persuade folks one way or the other.

One thing we might do to address Nico's comment about unintentionally using the
optional argument in CrOS-style syntax is make it so one can either operate on regions
*or* operate on a full-sized file, but not both at the same time. So for example, remove
the ability to do:
flashrom -p prog -i region:region.bin -r file.bin
flashrom -p prog -i region:region.bin -w file.bin

IIRC those were optimizations for ChromeOS factory scripts. Err'ing out in those cases
will address the concern Nico had about unintentionally using the optional argument,
though it will cost a few seconds on the manufacturing line.


Patchset:

PS2: 
  And then you
want to add a `-V` but miss the dash, it would still
 be accepted:
 
   $ flashrom -p prog --ifd -i bios:bios.bin -r V

 It's not very bad. IIRC, with the non-positional argument one
 could do much funnier things.
 
 Agree here! At least it seems to me that it just a concern for --read (which makes it
non-dangerous), since for the other two (-w/-v) it would be a lot of bad luck to have a
filename called V (still possible) while making the typo.
Flashrom will just err out if the file isn't the same size as the flash chip.

Actually, this use case is (was?) an optimization for parts of the manufacturing process
where we wanted to flash the final coreboot image and patch certain regions with data
generated at manufacturing time. Not very user-friendly, but it saved some time. I'd
be fine with err'ing out in upstream if folks think it's prone to misuse/errors.

  
  
 On the wiki[1], I once came up with this:
 
   [-i <region>]... (-r|-w|-v [<region>:]<filename>)...
 
 I think the complication with this approach is to support multiple file regions in a
single operation. We would have to enforce multiple -r without the possibility of mixes
between -r/-w/-v... Also, it means that we would have to move away form -i
<region>:<filename> (or as you suggested bellow - not document it).
Furthermore  -i would only make sense if there is a chip size operation since otherwise is
already embedded on the -r/-w/-v option. As an example of something that would not make
much sense:
 
   $ flashrom -i region1 -r region2:file # excluding a -r chip_file
Yeah, we should stick with one way to specify regions. Deprecating `-i` and only using
`-r|-w|-v [<region>:]<filename>` could work, though as you say we need to
ensure that only one type of operation is used for the entire command.


PS2: 
I'm not convinced that adding another syntax for operations involving regions is a net
benefit, but I won't stand in the way if you all think this is definitely an
improvement.

IMO if we're going to propose a new syntax, then let's think it through carefully
so that it handles all cases elegantly.



-- 
To view, visit https://review.coreboot.org/c/flashrom/+/30979
To unsubscribe, or for help writing mail filters, visit
https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Idfba11ec9991aac423b07f68f7dc45e7bebbb06b
Gerrit-Change-Number: 30979
Gerrit-PatchSet: 2
Gerrit-Owner: Nico Huber &lt;nico.h(a)gmx.de&gt;
Gerrit-Reviewer: Anastasia Klimchuk &lt;aklm(a)chromium.org&gt;
Gerrit-Reviewer: Angel Pons &lt;th3fanbus(a)gmail.com&gt;
Gerrit-Reviewer: Arthur Heymans &lt;arthur(a)aheymans.xyz&gt;
Gerrit-Reviewer: Carl-Daniel Hailfinger &lt;hailfinger(a)gmail.com&gt;
Gerrit-Reviewer: Daniel Campello &lt;campello(a)chromium.org&gt;
Gerrit-Reviewer: David Hendricks &lt;david.hendricks(a)gmail.com&gt;
Gerrit-Reviewer: Edward O'Callaghan &lt;quasisec(a)chromium.org&gt;
Gerrit-Reviewer: Nico Huber &lt;nico.h(a)gmx.de&gt;
Gerrit-Reviewer: Nikolai Artemiev &lt;nartemiev(a)google.com&gt;
Gerrit-Reviewer: Paul Menzel &lt;paulepanter(a)mailbox.org&gt;
Gerrit-Reviewer: Stefan T &lt;stefan.tauner(a)gmx.at&gt;
Gerrit-Reviewer: build bot (Jenkins) &lt;no-reply(a)coreboot.org&gt;
Gerrit-Attention: Nico Huber &lt;nico.h(a)gmx.de&gt;
Gerrit-Attention: Edward O'Callaghan &lt;quasisec(a)chromium.org&gt;
Gerrit-Attention: Daniel Campello &lt;campello(a)chromium.org&gt;
Gerrit-Attention: Angel Pons &lt;th3fanbus(a)gmail.com&gt;
Gerrit-Attention: Anastasia Klimchuk &lt;aklm(a)chromium.org&gt;
Gerrit-Attention: Nikolai Artemiev &lt;nartemiev(a)google.com&gt;
Gerrit-Comment-Date: Sun, 09 May 2021 01:05:59 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Nico Huber &lt;nico.h(a)gmx.de&gt;
Comment-In-Reply-To: David Hendricks &lt;david.hendricks(a)gmail.com&gt;
Comment-In-Reply-To: Edward O'Callaghan &lt;quasisec(a)chromium.org&gt;
Comment-In-Reply-To: Daniel Campello &lt;campello(a)chromium.org&gt;
Gerrit-MessageType: comment

Change in flashrom[master]: [RFC] cli_classic: Add `--mode (read|write|verify|erase)` parameter

[Daniel Campello (Code Review)]

Attention is currently required from: Nico Huber, David Hendricks, Angel Pons, Anastasia
Klimchuk, Nikolai Artemiev.
Daniel Campello has posted comments on this change. (
https://review.coreboot.org/c/flashrom/+/30979 )

Change subject: [RFC] cli_classic: Add `--mode (read|write|verify|erase)` parameter
......................................................................


Patch Set 2:

(1 comment)

Patchset:

PS2: 
  And then you want to add a `-V` but miss the dash, it
would still
 be accepted:
 
   $ flashrom -p prog --ifd -i bios:bios.bin -r V

 It's not very bad. IIRC, with the non-positional argument one
 could do much funnier things.
Agree here! At least it seems to me that it just a concern for --read (which makes it
non-dangerous), since for the other two (-w/-v) it would be a lot of bad luck to have a
filename called V (still possible) while making the typo.

  
 On the wiki[1], I once came up with this:
 
   [-i <region>]... (-r|-w|-v [<region>:]<filename>)...
I think the complication with this approach is to support multiple file regions in a
single operation. We would have to enforce multiple -r without the possibility of mixes
between -r/-w/-v... Also, it means that we would have to move away form -i
<region>:<filename> (or as you suggested bellow - not document it).
Furthermore  -i would only make sense if there is a chip size operation since otherwise is
already embedded on the -r/-w/-v option. As an example of something that would not make
much sense:

  $ flashrom -i region1 -r region2:file # excluding a -r chip_file

  
 [1] https://flashrom.org/Per_region_file_arguments#More_explicit_alternative
I probably should have clicked the link before starting replying as my concerns are
documented there :)



-- 
To view, visit https://review.coreboot.org/c/flashrom/+/30979
To unsubscribe, or for help writing mail filters, visit
https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Idfba11ec9991aac423b07f68f7dc45e7bebbb06b
Gerrit-Change-Number: 30979
Gerrit-PatchSet: 2
Gerrit-Owner: Nico Huber &lt;nico.h(a)gmx.de&gt;
Gerrit-Reviewer: Anastasia Klimchuk &lt;aklm(a)chromium.org&gt;
Gerrit-Reviewer: Angel Pons &lt;th3fanbus(a)gmail.com&gt;
Gerrit-Reviewer: Arthur Heymans &lt;arthur(a)aheymans.xyz&gt;
Gerrit-Reviewer: Carl-Daniel Hailfinger &lt;hailfinger(a)gmail.com&gt;
Gerrit-Reviewer: Daniel Campello &lt;campello(a)chromium.org&gt;
Gerrit-Reviewer: David Hendricks &lt;david.hendricks(a)gmail.com&gt;
Gerrit-Reviewer: Edward O'Callaghan &lt;quasisec(a)chromium.org&gt;
Gerrit-Reviewer: Nico Huber &lt;nico.h(a)gmx.de&gt;
Gerrit-Reviewer: Nikolai Artemiev &lt;nartemiev(a)google.com&gt;
Gerrit-Reviewer: Paul Menzel &lt;paulepanter(a)mailbox.org&gt;
Gerrit-Reviewer: Stefan T &lt;stefan.tauner(a)gmx.at&gt;
Gerrit-Reviewer: build bot (Jenkins) &lt;no-reply(a)coreboot.org&gt;
Gerrit-Attention: Nico Huber &lt;nico.h(a)gmx.de&gt;
Gerrit-Attention: David Hendricks &lt;david.hendricks(a)gmail.com&gt;
Gerrit-Attention: Angel Pons &lt;th3fanbus(a)gmail.com&gt;
Gerrit-Attention: Anastasia Klimchuk &lt;aklm(a)chromium.org&gt;
Gerrit-Attention: Nikolai Artemiev &lt;nartemiev(a)google.com&gt;
Gerrit-Comment-Date: Sat, 08 May 2021 14:09:42 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber &lt;nico.h(a)gmx.de&gt;
Comment-In-Reply-To: Daniel Campello &lt;campello(a)chromium.org&gt;
Gerrit-MessageType: comment