[M] Change in flashrom[main]: cli_classic.c: Make -r/-w/-v argument optional when using -i

[Anastasia Klimchuk (Code Review)]

Attention is currently required from: Matt DeVillier.

Anastasia Klimchuk has posted comments on this change by Matt DeVillier. ( https://review.coreboot.org/c/flashrom/+/85159?usp=email )

Change subject: cli_classic.c: Make -r/-w/-v argument optional when using -i
......................................................................

Patch Set 2:

(13 comments)

Commit Message:

https://review.coreboot.org/c/flashrom/+/85159/comment/8ab82ee6_0022e36e?usp... :
PS1, Line 7: Make -r/-w/-v argument optional
> Ack. […]

Done

Commit Message:

https://review.coreboot.org/c/flashrom/+/85159/comment/9f4b04e4_0a86b02c?usp... :
PS2, Line 16: this patch
            : essentially removes the requirement to provide an unused parameter
Oh I really like this part. And thank you for updating existing examples in the manpage (which were only mentioning write, but now you fixed it).

https://review.coreboot.org/c/flashrom/+/85159/comment/9931eee1_5648c8f5?usp... :
PS2, Line 21: TEST=verify read/write/verify operations successful when specifying
            : the filename only after the region.
As a note, you can just copy-paste the list of commands that you ran to test, might be easier than writing a text about what you have done.

But more importantly, if you could ran few tests without `-i` option, just a standard read, write, erase: 
- correct examples
- and also with missing file parameter, this should fail to validate the command line syntax (fail before starting the operation)

Running with dummyflasher is totally fine

File cli_classic.c:

https://review.coreboot.org/c/flashrom/+/85159/comment/d141bd1e_5d43c02f?usp... :
PS2, Line 691: if (read_buf_from_include_args(get_layout(flash), newcontents))
Should it be `else if`? We can't have both, right?

https://review.coreboot.org/c/flashrom/+/85159/comment/a56b13a4_7c4665f5?usp... :
PS2, Line 727: if (read_buf_from_include_args(get_layout(flash), newcontents))
Same question, should it be `else if`?

https://review.coreboot.org/c/flashrom/+/85159/comment/1de64943_4e02cc8a?usp... :
PS2, Line 1351: then:
              : 	 *     - Do partial read for each -i arg, creating a new file for
              : 	 *       each region where a filename is provided (-i region:filename).
              : 	 *     - Create a ROM-sized file with partially filled content. For each
              : 	 *       -i arg, fill the corresponding offset with content from ROM.
... then filename specified for `-r` is ignored?
The rest should be explained in preceding paragraph already, right?

https://review.coreboot.org/c/flashrom/+/85159/comment/c0ced021_396befe0?usp... :
PS2, Line 1357: Rules for writing and verifying:
I know it wasn't you who wrote these Rules initially, but let's make them more readable (it's hard right now).

Specific points:

1) Rules for reading, writing , verifying should have the same structure (currently not)
2) Make it as a list of all possible combinations:

- no filename specified -> error
- filename is specified for -r/-w/-v only
- filename is specified for -i only -> explain what happens (which is already written here I think, just scattered)
- filename is specified for both -r/-w/-v and -i -> -i takes priority, filename in -r/-w/-v is ignored.

It is likely that after writing this, you will copy-paste to manpage (in addition to what's there). So it's very useful to write clear text, and then manpage will be a self-service for users!

File doc/classic_cli_manpage.rst:

https://review.coreboot.org/c/flashrom/+/85159/comment/36e547a3_de1cb468?usp... :
PS2, Line 57: file
Let's do the same formatting as in the first sentence,

`**<file>**`

https://review.coreboot.org/c/flashrom/+/85159/comment/7f979c7e_767aa92b?usp... :
PS2, Line 70: file
Let's do the same formatting as in the first sentence,

`**<file>**`

https://review.coreboot.org/c/flashrom/+/85159/comment/77fa166a_f9dccbf1?usp... :
PS2, Line 100: file
Let's do the same formatting as in the first sentence,

`**<file>**`

https://review.coreboot.org/c/flashrom/+/85159/comment/fc9a6889_efed68d3?usp... :
PS2, Line 206: <file>
same here, same formatting

`**<file>**`

File include/layout.h:

https://review.coreboot.org/c/flashrom/+/85159/comment/ba4ff03c_3f0bc5d5?usp... :
PS2, Line 70: int register_include_arg(struct layout_include_args **, const char *arg);
            : int process_include_args(struct flashrom_layout *, const struct layout_include_args *);
            : int check_include_args_filename(const struct layout_include_args *);
            : void cleanup_include_args(struct layout_include_args **);
If you are in the good mood, maybe you would consider adding few tests for these functions?
We have some tests already for functions in layout.c, in tests/layout.c, it would be great to cover some of include args logic too.

I thought you might be interested in that processing of include args has tests and protected from breakage ;)

Doesn't need to be in this patch, can be a separate patch, any time later. Either way I would be so so happy to get few more tests! Thank you!

File layout.c:

https://review.coreboot.org/c/flashrom/+/85159/comment/c385b3f4_fee26836?usp... :
PS2, Line 296: Error: No region file specified
Maybe we can give more details, so that there no confusion for the users where the error comes from:
> Error: No region file specified for the --include/-i command.