Attention is currently required from: Matt DeVillier.

View Change

13 comments:

• Commit Message:

  • Patch Set #1, Line 7: Make -r/-w/-v argument optional

    Ack. […]

    Done

• Commit Message:

  • Patch Set #2, 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).

  • Patch Set #2, 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:

  • Patch Set #2, Line 691: if (read_buf_from_include_args(get_layout(flash), newcontents))

    Should it be `else if`? We can't have both, right?

  • Patch Set #2, Line 727: if (read_buf_from_include_args(get_layout(flash), newcontents))

    Same question, should it be `else if`?

  • Patch Set #2, 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?

  • Patch Set #2, 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:

  • Patch Set #2, Line 57: file

    Let's do the same formatting as in the first sentence,

    `**<file>**`

  • Patch Set #2, Line 70: file

    Let's do the same formatting as in the first sentence,

    `**<file>**`

  • Patch Set #2, Line 100: file

    Let's do the same formatting as in the first sentence,

    `**<file>**`

  • Patch Set #2, Line 206: <file>

    same here, same formatting

    `**<file>**`

• File include/layout.h:

  • Patch Set #2, 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:

  • Patch Set #2, 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.

To view, visit change 85159. To unsubscribe, or for help writing mail filters, visit settings.