Am 19.08.2012 03:29 schrieb Stefan Tauner:
On Sun, 19 Aug 2012 01:51:16 +0200 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
Counter-proposal (conversion from scratch). AFAICS the only differences to your version regarding code flow are in cli_classic.c and flashrom.c.
this is not a full review, i just looked at flashrom.c and cli_classic.c
Index: flashrom-flashctx_separate_struct_flashchip/cli_classic.c
--- flashrom-flashctx_separate_struct_flashchip/cli_classic.c (Revision 1576) +++ flashrom-flashctx_separate_struct_flashchip/cli_classic.c (Arbeitskopie) @@ -109,8 +109,8 @@ { unsigned long size; /* Probe for up to three flash chips. */
- const struct flashchip *flash;
- struct flashctx flashes[3];
- const struct flashchip *chip;
- struct flashctx flashes[3] = {};
this is not standard C afaik, but gcc does not accept (the standard) {0} without warning either, hence my {{0}} suggestion. http://stackoverflow.com/questions/1352370/c-static-array-initialization-how...
Thanks for the hint. We'll take your version.
struct flashctx *fill_flash; const char *name; int namelen, opt, i, j; @@ -389,17 +389,16 @@ } /* Does a chip with the requested name exist in the flashchips array? */ if (chip_to_probe) {
for (flash = flashchips; flash && flash->name; flash++)if (!strcmp(flash->name, chip_to_probe))
for (chip = flashchips; chip && chip->name; chip++)if (!strcmp(chip->name, chip_to_probe)) break;
if (!flash || !flash->name) {
if (!chip || !chip->name) { msg_cerr("Error: Unknown chip '%s' specified.\n", chip_to_probe); msg_gerr("Run flashrom -L to view the hardware supported in this flashrom version.\n"); ret = 1; goto out;
/* Clean up after the check. */flash = NULL;
/* Keep chip around for later usage. */nasty hack for the evil hack named forced reads. :)
Hm yes, I should have mentioned forced reads in that comment.
@@ -456,9 +452,15 @@ /* This loop just counts compatible controllers. */ for (j = 0; j < registered_programmer_count; j++) { pgm = ®istered_programmers[j];
if (pgm->buses_supported & flashes[0].bustype)
/* chip is still set from the chip_to_probe earlier in this function. */if (pgm->buses_supported & chip->bustype) compatible_programmers++; }if (!compatible_programmers) {msg_cinfo("No compatible controller found for the requested flash chip.\n");ret = 1;goto out_shutdown;}good catch!
Thanks!
Index: flashrom-flashctx_separate_struct_flashchip/flashrom.c
--- flashrom-flashctx_separate_struct_flashchip/flashrom.c (Revision 1576) +++ flashrom-flashctx_separate_struct_flashchip/flashrom.c (Arbeitskopie) @@ -950,44 +950,52 @@ return 1; }
-int probe_flash(struct registered_programmer *pgm, int startchip,
struct flashctx *fill_flash, int force)+int probe_flash(struct registered_programmer *pgm, int startchip, struct flashctx *flash, int force) {
- const struct flashchip *flash;
- const struct flashchip *chip; unsigned long base = 0; char location[64]; uint32_t size; enum chipbustype buses_common; char *tmp;
- for (flash = flashchips + startchip; flash && flash->name; flash++) {
if (chip_to_probe && strcmp(flash->name, chip_to_probe) != 0)
- for (chip = flashchips + startchip; chip && chip->name; chip++) {
if (chip_to_probe && strcmp(chip->name, chip_to_probe) != 0) continue;
buses_common = pgm->buses_supported & flash->bustype;
buses_common = pgm->buses_supported & chip->bustype;
flash->vendor, flash->name, flash->total_size);if (!flash->probe && !force) {
chip->vendor, chip->name, chip->total_size);this can be combined with the line before easily.
Right.
if (!flash->chip) {msg_gerr("Out of memory!\n");// FIXME: Is -1 the right return code?return -1;hm... well it breaks the outer loop, but there is no real error handling there. better add an exit(1) here (yes, really :) for now and fix it when the error definitions are merged (which has rather high priority for me). just returning with the current code would postpone the explosion to doit(). NB: i havent checked that too thoroughly.
Hm... while I think that we shouldn't introduce new exit(1), I agree that this case needs careful audit, and the patch is already complicated enough as is.
@@ -1022,15 +1030,18 @@ }
if (startchip == 0 ||
((fill_flash->model_id != GENERIC_DEVICE_ID) &&(fill_flash->model_id != SFDP_DEVICE_ID)))
((flash->chip->model_id != GENERIC_DEVICE_ID) &&(flash->chip->model_id != SFDP_DEVICE_ID)))i like that bit better in my patch, because it groups together what belongs together.
You mean having both related checks on one line? Or do you mean using chip-> instead of flash->chip-> ?
break;notfound:
programmer_unmap_flash_region((void *)fill_flash->virtual_memory, size);
programmer_unmap_flash_region((void *)flash->virtual_memory, size);flash->virtual_memory = (chipaddr)NULL;free(flash->chip);flash->chip = NULL;pretty clear indication that this function has issues imho :)
_Had_ issues. I really like the new code, because it not only frees/unmaps stuff, it also leaves no dangling pointers around.
those lines are a candidate to form a function flashctx_init or something like that, if we need this more often (i really hope we dont.)
}
- if (!flash || !flash->name)
- if (!flash->chip) return -1;
i wondered too why that ->name check was there, any ideas?
Yes. The old !flash check would only have triggered in very odd circumstances (empty array in flashchips.c), and !flash->name was the detection for end-of-array (last array member was zeroed). The new !flash->chip check is way more readable and doesn't rely on implicit properties of an array in another file.
How do we proceed? Should I repost a fixed version of my patch so you can merge yours and mine? Can you do the missing dediprog conversion locally or do you want to take mine?
Regards, Carl-Daniel