[flashrom] [PATCH] Partial write cleanups

[Carl-Daniel Hailfinger]

Avoid two memory leaks in doit() which were unproblematic for flashrom
because flashrom terminates after finishing doit().
Rename oldcontents to curconents in erase_and_write_block_helper().
Return write length from get_next_write() instead of filling it as
referenced parameter.

Thanks to Michael Karcher for pointing out the first two issues.

Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>

diff -u flashrom-partial_write_rolling_erase_write/flashrom.c flashrom-partial_write_rolling_erase_write_cleanup/flashrom.c
--- flashrom-partial_write_rolling_erase_write/flashrom.c	(Arbeitskopie)
+++ flashrom-partial_write_rolling_erase_write_cleanup/flashrom.c	(Arbeitskopie)
@@ -878,14 +878,11 @@
  * length of the chip.
  */
 static int get_next_write(uint8_t *have, uint8_t *want, int len,
-			  int *first_start, int *first_len,
-			  enum write_granularity gran)
+			  int *first_start, enum write_granularity gran)
 {
-	int result = 0, rel_start = 0;
+	int result = 0, rel_start = 0, first_len = 0;
 	int i, limit;
 
-	/* The passed in variable might be uninitialized. */
-	*first_len = 0;
 	switch (gran) {
 	case write_gran_1bit:
 	case write_gran_1byte:
@@ -903,14 +900,14 @@
 					/* First location where have and want
 					 * do not differ anymore.
 					 */
-					*first_len = i - rel_start;
+					first_len = i - rel_start;
 					break;
 				}
 			}
 		}
 		/* Did the loop terminate without setting first_len? */
-		if (result && ! *first_len)
-			*first_len = i - rel_start;
+		if (result && ! first_len)
+			first_len = i - rel_start;
 		break;
 	case write_gran_256bytes:
 		for (i = 0; i < len / 256; i++) {
@@ -929,21 +926,21 @@
 					/* First location where have and want
 					 * do not differ anymore.
 					 */
-					*first_len = i * 256 - rel_start;
+					first_len = i * 256 - rel_start;
 					break;
 				}
 			}
 		}
 		/* Did the loop terminate without setting first_len? */
-		if (result && ! *first_len)
-			*first_len = min(i * 256 - rel_start, len);
+		if (result && ! first_len)
+			first_len = min(i * 256 - rel_start, len);
 		break;
 	default:
 		msg_cerr("%s: Unsupported granularity! Please report a bug at "
 			 "flashrom at flashrom.org\n", __func__);
 	}
 	*first_start += rel_start;
-	return result;
+	return first_len;
 }
 
 /* This function generates various test patterns useful for testing controller
@@ -1370,7 +1367,7 @@
 
 static int erase_and_write_block_helper(struct flashchip *flash,
 					unsigned int start, unsigned int len,
-					uint8_t *oldcontents,
+					uint8_t *curcontents,
 					uint8_t *newcontents,
 					int (*erasefn) (struct flashchip *flash,
 							unsigned int addr,
@@ -1383,26 +1380,25 @@
 	int writecount = 0;
 	enum write_granularity gran = write_gran_256bytes; /* FIXME */
 
-	/* oldcontents and newcontents are opaque to walk_eraseregions, and
+	/* curcontents and newcontents are opaque to walk_eraseregions, and
 	 * need to be adjusted here to keep the impression of proper abstraction
 	 */
-	oldcontents += start;
+	curcontents += start;
 	newcontents += start;
 	msg_cdbg(":");
 	/* FIXME: Assume 256 byte granularity for now to play it safe. */
-	if (need_erase(oldcontents, newcontents, len, gran)) {
+	if (need_erase(curcontents, newcontents, len, gran)) {
 		msg_cdbg("E");
 		ret = erasefn(flash, start, len);
 		if (ret)
 			return ret;
-		/* Erase was successful. Adjust oldcontents. */
-		memset(oldcontents, 0xff, len);
+		/* Erase was successful. Adjust curcontents. */
+		memset(curcontents, 0xff, len);
 		skip = 0;
 	}
-	while (get_next_write(oldcontents + starthere,
-			      newcontents + starthere,
-			      len - starthere, &starthere,
-			      &lenhere, gran)) {
+	while ((lenhere = get_next_write(curcontents + starthere,
+					 newcontents + starthere,
+					 len - starthere, &starthere, gran))) {
 		if (!writecount++)
 			msg_cdbg("W");
 		/* Needs the partial write function signature. */
@@ -1796,8 +1792,8 @@
 
 	if (chip_safety_check(flash, force, filename, read_it, write_it, erase_it, verify_it)) {
 		msg_cerr("Aborting.\n");
-		programmer_shutdown();
-		return 1;
+		ret = 1;
+		goto out_nofree;
 	}
 
 	/* Given the existence of read locks, we want to unlock for read,
@@ -1808,8 +1804,7 @@
 
 	if (read_it) {
 		ret = read_flash_to_file(flash, filename);
-		programmer_shutdown();
-		return ret;
+		goto out_nofree;
 	}
 
 	oldcontents = (uint8_t *) malloc(size);
@@ -1835,14 +1830,13 @@
 			emergency_help_message();
 			ret = 1;
 		}
-		programmer_shutdown();
-		return ret;
+		goto out;
 	}
 
 	if (write_it || verify_it) {
 		if (read_buf_from_file(newcontents, size, filename)) {
-			programmer_shutdown();
-			return 1;
+			ret = 1;
+			goto out;
 		}
 
 #if CONFIG_INTERNAL == 1
@@ -1859,8 +1853,8 @@
 	 */
 	msg_cdbg("Reading old flash chip contents...\n");
 	if (flash->read(flash, oldcontents, 0, size)) {
-		programmer_shutdown();
-		return 1;
+		ret = 1;
+		goto out;
 	}
 
 	// This should be moved into each flash part's code to do it 
@@ -1878,13 +1872,13 @@
 					msg_cinfo("Good. It seems nothing was "
 						  "changed.\n");
 					nonfatal_help_message();
-					programmer_shutdown();
-					return 1;
+					ret = 1;
+					goto out;
 				}
 			}
 			emergency_help_message();
-			programmer_shutdown();
-			return 1;
+			ret = 1;
+			goto out;
 		}
 	}
 
@@ -1901,6 +1895,9 @@
 	}
 
+out:
+	free(oldcontents);
+	free(newcontents);
+out_nofree:
 	programmer_shutdown();
-
 	return ret;
 }


-- 
http://www.hailfinger.org/