Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40325 )
Change subject: Add writeprotect support infrastructure ......................................................................
Patch Set 8:
(6 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?
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)
https://review.coreboot.org/c/flashrom/+/40325/8/cli_classic.c@639 PS8, Line 639: Please, no space here
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.org/Development_Guidelines#Coding_style
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.
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.