[flashrom-gerrit] Change in flashrom[master]: Add writeprotect support infrastructure

[Edward O'Callaghan (Code Review)]

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