Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40325 )
Change subject: Add writeprotect support infrastructure ......................................................................
Patch Set 8:
(9 comments)
https://review.coreboot.org/c/flashrom/+/40325/8/cli_classic.c File cli_classic.c:
https://review.coreboot.org/c/flashrom/+/40325/8/cli_classic.c@115 PS8, Line 115: char *endptr = NULL, *token = NULL; : : if (!optarg) { : msg_gerr("Error: No wp-range values provided\n"); : return -1; : } : : token = strtok(optarg, ","); : if (!token) { : msg_gerr("Error: Invalid wp-range argument format\n"); : return -1; : } : *start = strtoul(token, &endptr, 0); : : token = strtok(NULL, ","); : if (!token) { : msg_gerr("Error: Invalid wp-range argument format\n"); : return -1; : } : *len = strtoul(token, &endptr, 0); : : return 0;
One tab too many?
Done
https://review.coreboot.org/c/flashrom/+/40325/8/cli_classic.c@153 PS8, Line 153: set_wp_disable = 0, wp_status = 0, wp_list = 0;
Missing `int` (yes, I know it can work like this, but please be consistent)
Done
https://review.coreboot.org/c/flashrom/+/40325/8/cli_classic.c@639 PS8, Line 639:
Please, no space here
Done
https://review.coreboot.org/c/flashrom/+/40325/8/cli_classic.c@671 PS8, Line 671: "on this flash chip.\n");
Please never break strings. It kills grep-ability. https://flashrom. […]
Done
https://review.coreboot.org/c/flashrom/+/40325/8/cli_classic.c@682 PS8, Line 682: r("Error: write protect is not supported " : "on this flash chip.\n"); also fixed.
https://review.coreboot.org/c/flashrom/+/40325/8/cli_classic.c@706 PS8, Line 706: msg_gerr("Error: write protect is not supported " : "on this flash chip.\n"); also fixed.
https://review.coreboot.org/c/flashrom/+/40325/8/cli_classic.c@718 PS8, Line 718: msg_gerr("Error: write protect is not supported " : "on this flash chip.\n"); : ret = 1; also fixed.
https://review.coreboot.org/c/flashrom/+/40325/8/cli_classic.c@726 PS8, Line 726: if (set_wp_range && set_wp_region) {
This can be handled much earlier, around where `set_wp_enable && set_wp_disable` is checked.
Done and simplified the logic as well, thanks.
https://review.coreboot.org/c/flashrom/+/40325/8/writeprotect.c File writeprotect.c:
https://review.coreboot.org/c/flashrom/+/40325/8/writeprotect.c@188 PS8, Line 188: msg_cerr("error setting modifier "
Another string that should not be broken.
Done