Attention is currently required from: Nico Huber, Edward O'Callaghan, Daniel Campello, Angel Pons, Anastasia Klimchuk, Nikolai Artemiev.

Patch set 2:Code-Review -1

View Change

3 comments:

• Commit Message:

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

  • Patch Set #2:

    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.

  • Patch Set #2:

    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 change 30979. To unsubscribe, or for help writing mail filters, visit settings.