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 
---
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)

[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.

[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 
---
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

[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.

[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,
```

[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

[Angel Pons (Code Review)]

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

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

Abandoned