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@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@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; }
Am Mittwoch, den 03.11.2010, 03:51 +0100 schrieb Carl-Daniel Hailfinger:
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@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;
As "result" no longer is the return value, please rename the variable. Something like "write_needed" would make sense, but...
@@ -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; } }
if (result && ! *first_len)*first_len = i - rel_start;
if (result && ! first_len)first_len = i - rel_start;
... you could also simplify this stuff by having a variable "in_write_region" instead of result, which you clear in the block where you set "first_len", and then the "if" would just need to check the "in_write_region" flag instead of combining two things.
Or, wait a moment! Do you see some similarity between the assignment in the if and the assignment before the break? Yes! The expressions are identical. So: remove the first_len assignment before the break, you also don't need to clear a flag in that block, just break. And then you do the first_len assignment if the variable till now known as result is set. The logic is that the loop always terminates at the end of the region to write. Which might either be because of the loop end condition or the matching compare - who cares, actually.
No Ack yet as I want to see the result if incorporating my comments into this function before.
+out:
- free(oldcontents);
- free(newcontents);
+out_nofree: programmer_shutdown();
if you initialize oldcontents and newcontents with NULL, free is guaranteed by ANSI C to be a no-op, so you might get rid of the out_nofree label. Whether one wants to make use of that feature or considers not calling free at all if there was no malloc is a matter of taste.
Regards, Michael Karcher
On 03.11.2010 23:44, Michael Karcher wrote:
Am Mittwoch, den 03.11.2010, 03:51 +0100 schrieb Carl-Daniel Hailfinger:
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@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;
As "result" no longer is the return value, please rename the variable. Something like "write_needed" would make sense, but...
@@ -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; } }
if (result && ! *first_len)*first_len = i - rel_start;
if (result && ! first_len)first_len = i - rel_start;... you could also simplify this stuff by having a variable "in_write_region" instead of result, which you clear in the block where you set "first_len", and then the "if" would just need to check the "in_write_region" flag instead of combining two things.
Or, wait a moment! Do you see some similarity between the assignment in the if and the assignment before the break? Yes! The expressions are identical. So: remove the first_len assignment before the break, you also don't need to clear a flag in that block, just break. And then you do the first_len assignment if the variable till now known as result is set. The logic is that the loop always terminates at the end of the region to write. Which might either be because of the loop end condition or the matching compare - who cares, actually.
I actually had that cleanup included in an earlier partial write patch, but decided to scratch it because I was still searching for the bug in the partial write patch back then. And once the partial write patch started to work for everyone, I was hesitant to reintroduce the cleanup and thus invalidate all tests. Now that partial write is merged and we know it works, I may as well merge that cleanup again.
No Ack yet as I want to see the result if incorporating my comments into this function before.
+out:
- free(oldcontents);
- free(newcontents);
+out_nofree: programmer_shutdown();
if you initialize oldcontents and newcontents with NULL, free is guaranteed by ANSI C to be a no-op, so you might get rid of the out_nofree label. Whether one wants to make use of that feature or considers not calling free at all if there was no malloc is a matter of taste.
That's a good point. I believe we don't handle this consistently in flashrom. The Linux kernel usually uses out: and out_free: labels, and IIRC that applies even to cases where free() would be harmless, but it's been a while since I read huge chunks of kernel code, so that may have changed.
Regards, Carl-Daniel
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(). Unify the code for all granularities in get_next_write(). 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. Thanks to David Hendricks for pointing out the third issue and suggesting a way to unify that code.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: flashrom-partial_write_rolling_erase_write_cleanup/flashrom.c =================================================================== --- flashrom-partial_write_rolling_erase_write_cleanup/flashrom.c (Revision 1224) +++ flashrom-partial_write_rolling_erase_write_cleanup/flashrom.c (Arbeitskopie) @@ -869,81 +869,62 @@ * @want buffer with desired content * @len length of the checked area * @gran write granularity (enum, not count) - * @return 0 if no write is needed, 1 otherwise - * @first_start offset of the first byte which needs to be written - * @first_len length of the first contiguous area which needs to be written + * @first_start offset of the first byte which needs to be written (passed in + * value is increased by the offset of the first needed write + * relative to have/want or unchanged if no write is needed) + * @return length of the first contiguous area which needs to be written + * 0 if no write is needed * * FIXME: This function needs a parameter which tells it about coalescing * in relation to the max write length of the programmer and the max write * 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 i, limit; + int need_write = 0, rel_start = 0, first_len = 0; + int i, limit, stride;
- /* The passed in variable might be uninitialized. */ - *first_len = 0; switch (gran) { case write_gran_1bit: case write_gran_1byte: - for (i = 0; i < len; i++) { - if (have[i] != want[i]) { - if (!result) { - /* First location where have and want - * differ. - */ - result = 1; - rel_start = i; - } - } else { - if (result) { - /* First location where have and want - * do not differ anymore. - */ - *first_len = i - rel_start; - break; - } - } - } - /* Did the loop terminate without setting first_len? */ - if (result && ! *first_len) - *first_len = i - rel_start; + stride = 1; break; case write_gran_256bytes: - for (i = 0; i < len / 256; i++) { - limit = min(256, len - i * 256); - /* Are 'have' and 'want' identical? */ - if (memcmp(have + i * 256, want + i * 256, limit)) { - if (!result) { - /* First location where have and want - * differ. - */ - result = 1; - rel_start = i * 256; - } - } else { - if (result) { - /* First location where have and want - * do not differ anymore. - */ - *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); + stride = 256; break; default: msg_cerr("%s: Unsupported granularity! Please report a bug at " "flashrom@flashrom.org\n", __func__); + /* Claim that no write was needed. A write with unknown + * granularity is too dangerous to try. + */ + return 0; } + for (i = 0; i < len / stride; i++) { + limit = min(stride, len - i * stride); + /* Are 'have' and 'want' identical? */ + if (memcmp(have + i * stride, want + i * stride, limit)) { + if (!need_write) { + /* First location where have and want differ. */ + need_write = 1; + rel_start = i * stride; + } + } else { + if (need_write) { + /* First location where have and want + * do not differ anymore. + */ + first_len = i * stride - rel_start; + break; + } + } + } + /* Did the loop terminate without setting first_len? */ + if (need_write && ! first_len) + first_len = min(i * stride - rel_start, len); *first_start += rel_start; - return result; + return first_len; }
/* This function generates various test patterns useful for testing controller @@ -1370,7 +1351,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 +1364,26 @@ 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)) { + /* get_next_write() sets starthere to a new value after the call. */ + 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 +1777,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 +1789,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 +1815,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 +1838,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 +1857,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; } }
@@ -1900,7 +1879,10 @@ emergency_help_message(); }
+out: + free(oldcontents); + free(newcontents); +out_nofree: programmer_shutdown(); - return ret; }
Am Donnerstag, den 04.11.2010, 05:20 +0100 schrieb Carl-Daniel Hailfinger:
Or, wait a moment! Do you see some similarity between the assignment in the if and the assignment before the break? Yes! The expressions are identical. So: remove the first_len assignment before the break, you also don't need to clear a flag in that block, just break. And then you do the first_len assignment if the variable till now known as result is set. The logic is that the loop always terminates at the end of the region to write. Which might either be because of the loop end condition or the matching compare - who cares, actually.
I actually had that cleanup included in an earlier partial write patch, but decided to scratch it because I was still searching for the bug in the partial write patch back then.
The special suggested modification is not in, but I like the new version already much better.
Acked-by: Michael Karcher flashrom@mkarcher.dialup.fu-berlin.de
Regards, Michael Karcher
On 05.11.2010 08:53, Michael Karcher wrote:
Am Donnerstag, den 04.11.2010, 05:20 +0100 schrieb Carl-Daniel Hailfinger:
Or, wait a moment! Do you see some similarity between the assignment in the if and the assignment before the break? Yes! The expressions are identical. So: remove the first_len assignment before the break, you also don't need to clear a flag in that block, just break. And then you do the first_len assignment if the variable till now known as result is set. The logic is that the loop always terminates at the end of the region to write. Which might either be because of the loop end condition or the matching compare - who cares, actually.
I actually had that cleanup included in an earlier partial write patch, but decided to scratch it because I was still searching for the bug in the partial write patch back then.
The special suggested modification is not in, but I like the new version already much better.
My bad, I had misunderstood your original review. Will post a patch on top of this which addresses the issue you mentioned.
Acked-by: Michael Karcher flashrom@mkarcher.dialup.fu-berlin.de
Thanks, committed in r1225.
Regards, Carl-Daniel
-
Carl-Daniel Hailfinger -
Michael Karcher