Change in flashrom[master]: Introduce `extract_programmer_param_toggle` helper

[Angel Pons (Code Review)]

Angel Pons has uploaded this change for review. (
https://review.coreboot.org/c/flashrom/+/46717 )


Change subject: Introduce `extract_programmer_param_toggle` helper
......................................................................

Introduce `extract_programmer_param_toggle` helper

This is useful to retrieve programmer params which can only be 0 or 1.

Change-Id: I2a27f36b0fbafc2113387ad82321f3d026811f5e
Signed-off-by: Angel Pons <th3fanbus(a)gmail.com>
---
M flashrom.c
M programmer.h
M realtek_mst_i2c_spi.c
3 files changed, 45 insertions(+), 27 deletions(-)



  git pull ssh://review.coreboot.org:29418/flashrom refs/changes/17/46717/1

diff --git a/flashrom.c b/flashrom.c
index c18a04f..d574902 100644
--- a/flashrom.c
+++ b/flashrom.c
@@ -789,6 +789,39 @@
 	return extract_param(&programmer_param, param_name, ",");
 }
 
+/*
+ * Extract a 'toggle' (on/off) value from a programmer parameter.
+ * Returns 0 on success, -1 on bad usage (in code or at runtime).
+ * If the parameter was not specified, 'value' remains unchanged.
+ */
+int extract_programmer_param_toggle(const char *param_name, int *value)
+{
+	if (!param_name || !value)
+		return -1;
+
+	char *param_str = extract_programmer_param(param_name);
+
+	if (!param_str)
+		return 0;
+
+	const char c = param_str[0];
+
+	free(param_str);
+
+	if (c == '1') {
+		*value = 1;
+		return 0;
+	}
+
+	if (c == '0') {
+		*value = 0;
+		return 0;
+	}
+
+	msg_perr("Incorrect toggle parameter format, %s = 1 or 0.\n", param_name);
+	return -1;
+}
+
 /* Returns the number of well-defined erasers for a chip. */
 static unsigned int count_usable_erasers(const struct flashctx *flash)
 {
diff --git a/programmer.h b/programmer.h
index 9a7892d..34aac88 100644
--- a/programmer.h
+++ b/programmer.h
@@ -612,6 +612,7 @@
 extern unsigned long flashbase;
 unsigned int count_max_decode_exceedings(const struct flashctx *flash);
 char *extract_programmer_param(const char *param_name);
+int extract_programmer_param_toggle(const char *param_name, int *value);
 
 /* spi.c */
 #define MAX_DATA_UNSPECIFIED 0
diff --git a/realtek_mst_i2c_spi.c b/realtek_mst_i2c_spi.c
index 538b07a..574087b 100644
--- a/realtek_mst_i2c_spi.c
+++ b/realtek_mst_i2c_spi.c
@@ -434,7 +434,7 @@
 
 static int get_params(int *i2c_bus, int *reset, int *enter_isp)
 {
-	char *bus_str = NULL, *reset_str = NULL, *isp_str = NULL;
+	char *bus_str = NULL;
 	int ret = SPI_GENERIC_ERROR;
 
 	bus_str = extract_programmer_param("bus");
@@ -464,33 +464,17 @@
 		msg_perr("%s: Bus number not specified.\n", __func__);
 	}
 
-	reset_str = extract_programmer_param("reset-mcu");
-	if (reset_str) {
-		if (reset_str[0] == '1')
-			*reset = 1;
-		else if (reset_str[0] == '0')
-			*reset = 0;
-		else {
-			msg_perr("%s: Incorrect param format, reset-mcu=1 or 0.\n", __func__);
-			ret = SPI_GENERIC_ERROR;
-		}
-	} else
-		*reset = 0; /* Default behaviour is no MCU reset on tear-down. */
-	free(reset_str);
+	/* Default behaviour is no MCU reset on tear-down. */
+	*reset = 0;
+	ret = extract_programmer_param_toggle("reset-mcu", reset);
+	if (ret)
+		goto _get_params_failed;
 
-	isp_str = extract_programmer_param("enter-isp");
-	if (isp_str) {
-		if (isp_str[0] == '1')
-			*enter_isp = 1;
-		else if (isp_str[0] == '0')
-			*enter_isp = 0;
-		else {
-			msg_perr("%s: Incorrect param format, enter-isp=1 or 0.\n", __func__);
-			ret = SPI_GENERIC_ERROR;
-		}
-	} else
-		*enter_isp = 1; /* Default behaviour is enter ISP on setup. */
-	free(isp_str);
+	/* Default behaviour is enter ISP on setup. */
+	*enter_isp = 1;
+	ret = extract_programmer_param_toggle("enter-isp", enter_isp);
+	if (ret)
+		goto _get_params_failed;
 
 _get_params_failed:
 	if (bus_str)

-- 
To view, visit https://review.coreboot.org/c/flashrom/+/46717
To unsubscribe, or for help writing mail filters, visit
https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I2a27f36b0fbafc2113387ad82321f3d026811f5e
Gerrit-Change-Number: 46717
Gerrit-PatchSet: 1
Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-MessageType: newchange

[Shiyu Sun (Code Review)]

Shiyu Sun has posted comments on this change. (
https://review.coreboot.org/c/flashrom/+/46717 )

Change subject: Introduce `extract_programmer_param_toggle` helper
......................................................................


Patch Set 2: Code-Review+1


-- 
To view, visit https://review.coreboot.org/c/flashrom/+/46717
To unsubscribe, or for help writing mail filters, visit
https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I2a27f36b0fbafc2113387ad82321f3d026811f5e
Gerrit-Change-Number: 46717
Gerrit-PatchSet: 2
Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Shiyu Sun <sshiyu(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-CC: Sam McNally <sammc(a)google.com>
Gerrit-Comment-Date: Mon, 26 Oct 2020 14:28:03 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[Edward O'Callaghan (Code Review)]

Edward O'Callaghan has posted comments on this change. (
https://review.coreboot.org/c/flashrom/+/46717 )

Change subject: Introduce `extract_programmer_param_toggle` helper
......................................................................


Patch Set 2:

(2 comments)

https://review.coreboot.org/c/flashrom/+/46717/1/flashrom.c 
File flashrom.c:

https://review.coreboot.org/c/flashrom/+/46717/1/flashrom.c@811 
PS1, Line 811: 	if (c == '1') {
             : 		*value = 1;
             : 		return 0;
             : 	}
             : 
             : 	if (c == '0') {
             : 		*value = 0;
             : 		return 0;
             : 	}
  I don't think a switch statement would help. […]
np. My mind was leaning towards 'true' and 'false' maybe need to be
handled as well for other spi masters to make full use of this across the tree. We also
wind up with a much more consistent param format for each using your solution. Feel free
to just ACK of course, the common theme here is to realise the full benefits of this
work.


https://review.coreboot.org/c/flashrom/+/46717/1/programmer.h 
File programmer.h:

https://review.coreboot.org/c/flashrom/+/46717/1/programmer.h@615 
PS1, Line 615: int extract_programmer_param_toggle(const char *param_name, int *value);
  I can convert other files in follow-ups (I don't
want this change to grow a lot). […]
In regards to the former point, I would argue
to split the change into two parts the first that introduces the helper (ideally with a
basic test) and the second that switches all the relevant spi masters to the new helper
framework atomically. It is much better than these ad hoc framework changes and lines up
with the advise you gave me about getting the write-protect framework right step by step
before merge.

In regards to unit-tests, the latter point about preventing software rot is precisely the
point of the unit-test. Unit-tests are typically a poor choice of tool for theorem proving
an initial implementation as you correctly eluded to. The unit-test could just be a simple
few cases, calling the fn iterated over an array of param combos and checking the return
value:

```
 i)   (NULL, NULL) => -1,
 ii)  ("str", NULL) => -1,
 iii) (NULL, &[int x]) => -1,
 iv)  ("p=", &[int x]) => 0?,
 v)   ("p=1", &[int x]) => {0 && x=1},
 vi)  ("p=0", &[int x]) => {0 && x=0},
 vii) ("p=o", &[int x]) => -1,
```



-- 
To view, visit https://review.coreboot.org/c/flashrom/+/46717
To unsubscribe, or for help writing mail filters, visit
https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I2a27f36b0fbafc2113387ad82321f3d026811f5e
Gerrit-Change-Number: 46717
Gerrit-PatchSet: 2
Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Shiyu Sun <sshiyu(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-CC: Sam McNally <sammc(a)google.com>
Gerrit-Comment-Date: Sun, 25 Oct 2020 00:49:25 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org>
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-MessageType: comment

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. (
https://review.coreboot.org/c/flashrom/+/46717 )

Change subject: Introduce `extract_programmer_param_toggle` helper
......................................................................


Patch Set 2:

(3 comments)

https://review.coreboot.org/c/flashrom/+/46717/1/flashrom.c 
File flashrom.c:

https://review.coreboot.org/c/flashrom/+/46717/1/flashrom.c@803 
PS1, Line 803: 
  delete spurious \n
Strictly speaking, it's
not spurious, since I decided to add it here. But I've dropped it.

Please don't get me wrong, though: I'm not angry about it at all, it's just
that I couldn't care less about it. I recently decided that minor nits don't
deserve anyone's time to be wasted on. Given that code style is something nearly
everyone can comment on, and that making everyone happy about it is impossible, trying to
argue about it only inflicts pain and misery upon the involved. Thus, I now address these
situations and move on as quickly as possible.

And yes, writing this took me a while, but I'm Weird and I'd rather explain myself
than risk being misunderstood. :)

Done


https://review.coreboot.org/c/flashrom/+/46717/1/flashrom.c@811 
PS1, Line 811: 	if (c == '1') {
             : 		*value = 1;
             : 		return 0;
             : 	}
             : 
             : 	if (c == '0') {
             : 		*value = 0;
             : 		return 0;
             : 	}
  seems like a switch would make this tidy?
I
don't think a switch statement would help. Currently, there's only two cases to
handle, so using a switch statement doesn't make much of a difference.

If more possible values get added later on, then one would have to update the error
message below accordingly. If using a switch statement, one would need to write down the
possible cases twice: once for the switch statement, and once for the error message.


https://review.coreboot.org/c/flashrom/+/46717/1/programmer.h 
File programmer.h:

https://review.coreboot.org/c/flashrom/+/46717/1/programmer.h@615 
PS1, Line 615: int extract_programmer_param_toggle(const char *param_name, int *value);
  I like this but I feel flashrom has suffered from many
half done solutions in the past. […]
I can convert other files in follow-ups (I
don't want this change to grow a lot).

Regarding unit tests, I'm not sure how I would write tests for this function. I
believe I've handled all the possible scenarios that this function could run into, so
any test cases I'd come up with would be testing the same things I've accounted
for. While that would help prevent software rot, it would not handle bugs in the original
implementation.



-- 
To view, visit https://review.coreboot.org/c/flashrom/+/46717
To unsubscribe, or for help writing mail filters, visit
https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I2a27f36b0fbafc2113387ad82321f3d026811f5e
Gerrit-Change-Number: 46717
Gerrit-PatchSet: 2
Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Shiyu Sun <sshiyu(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Sam McNally <sammc(a)google.com>
Gerrit-Comment-Date: Sat, 24 Oct 2020 14:30:46 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-MessageType: comment

[Angel Pons (Code Review)]

Hello build bot (Jenkins), Shiyu Sun, Edward O'Callaghan, 

I'd like you to reexamine a change. Please visit

    https://review.coreboot.org/c/flashrom/+/46717

to look at the new patch set (#2).

Change subject: Introduce `extract_programmer_param_toggle` helper
......................................................................

Introduce `extract_programmer_param_toggle` helper

This is useful to retrieve programmer params which can only be 0 or 1.

Change-Id: I2a27f36b0fbafc2113387ad82321f3d026811f5e
Signed-off-by: Angel Pons <th3fanbus(a)gmail.com>
---
M flashrom.c
M programmer.h
M realtek_mst_i2c_spi.c
3 files changed, 44 insertions(+), 27 deletions(-)


  git pull ssh://review.coreboot.org:29418/flashrom refs/changes/17/46717/2
-- 
To view, visit https://review.coreboot.org/c/flashrom/+/46717
To unsubscribe, or for help writing mail filters, visit
https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I2a27f36b0fbafc2113387ad82321f3d026811f5e
Gerrit-Change-Number: 46717
Gerrit-PatchSet: 2
Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Shiyu Sun <sshiyu(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Sam McNally <sammc(a)google.com>
Gerrit-MessageType: newpatchset

[Edward O'Callaghan (Code Review)]

Edward O'Callaghan has posted comments on this change. (
https://review.coreboot.org/c/flashrom/+/46717 )

Change subject: Introduce `extract_programmer_param_toggle` helper
......................................................................


Patch Set 1: Code-Review+2

(3 comments)

https://review.coreboot.org/c/flashrom/+/46717/1/flashrom.c 
File flashrom.c:

https://review.coreboot.org/c/flashrom/+/46717/1/flashrom.c@803 
PS1, Line 803: 
delete spurious \n


https://review.coreboot.org/c/flashrom/+/46717/1/flashrom.c@811 
PS1, Line 811: 	if (c == '1') {
             : 		*value = 1;
             : 		return 0;
             : 	}
             : 
             : 	if (c == '0') {
             : 		*value = 0;
             : 		return 0;
             : 	}
seems like a switch would make this tidy?


https://review.coreboot.org/c/flashrom/+/46717/1/programmer.h 
File programmer.h:

https://review.coreboot.org/c/flashrom/+/46717/1/programmer.h@615 
PS1, Line 615: int extract_programmer_param_toggle(const char *param_name, int *value);
I like this but I feel flashrom has suffered from many half done solutions in the past. So
I feel like we should switch all users in one go. It's a little more work but I think
your solution can get its benefits fully realized with the effort. mstarddc_spi.c is one
example candidate, digilent_spi.c is another. Maybe buspirate_spi.c ?

I would very much like to see a unit-test written, do you have any thoughts and
suggestions about that?

Bigger vision here: Another step would be to perhaps have a struct type defined that
contains the param type and a union between of the primitive types so that common parse
logic could be co-located out of the spi masters here. This could then become part of the
libflashrom api as well.



-- 
To view, visit https://review.coreboot.org/c/flashrom/+/46717
To unsubscribe, or for help writing mail filters, visit
https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I2a27f36b0fbafc2113387ad82321f3d026811f5e
Gerrit-Change-Number: 46717
Gerrit-PatchSet: 1
Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Shiyu Sun <sshiyu(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Sat, 24 Oct 2020 13:26:38 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment