Am 19.08.2012 03:29 schrieb Stefan Tauner:
> On Sun, 19 Aug 2012 01:51:16 +0200
> Carl-Daniel Hailfinger <[email protected]> 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-verbose-do-i-need-to-be/1352379#1352379
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;
>> if (!buses_common)
>> continue;
>> msg_gdbg("Probing for %s %s, %d kB: ",
>> - 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
--
http://www.hailfinger.org/
_______________________________________________
flashrom mailing list
[email protected]
http://www.flashrom.org/mailman/listinfo/flashrom
