Attention is currently required from: Nico Huber, Paul Menzel, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk, Sergii Dmytruk.

View Change

1 comment:

• File cli_classic.c:

  • Patch Set #17, Line 220:

    		ret = flashrom_wp_set_range(flash, &cfg, &range);
    		if (ret) {
    			msg_gerr("The chip does not support the requested range.\n");
    			return ret;
    		}
    
    		/* Write range before other operations (i.e. enabling HW protection) */
    		ret = write_wp_config(flash, &cfg);
    		if (ret)
    			return ret;
    

    I understand we need to print extra messages here. To achieve that, flashrom_wp_set_range need to return error codes, can be 1,2,3 etc. Here, we can print messages depending on error code.

    I thought return codes could be a good option too, but there were other issues with the design and ultimately I thought the approach of separate read/modify/write functions was nicer, but there could be a better option. The main problems were:

    • If writing the config fails, the CLI should print some kind of helpful error message. Right now this involves converting the expected chip state and actual chip state to strings and printing them (roughly lines 175-185). If we put the verification logic in writeprotect.c, we have to pass back the expected/actual config objects so the CLI code can print them. That's doable but a bit messy.
    • If we call write_wp_config() from the set_range() and set_mode() then we would also have to pass the expected/actual config objects there as well.

    The API in its current form allows fetching state, setting both mode and range and writing it in one go. If it's changed to perform writes on every operation and you incorrectly set mode before the range, setting range might fail (possibly permanently). Current design seems safer: you make changes, handle errors caught by the library and only then try to change state of the chip.

    The usual intention is that a user would try to set the range (read/set_range/write), check that it succeeded, and only then set the mode (read/set_mode/write). So we could possibly change the set_mode and set_range functions to also write to the chip, but there are still the problems.

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