On Tue, 24 May 2011 09:01:13 +0200 Carl-Daniel Hailfinger <[email protected]> wrote:
> Am 24.05.2011 02:21 schrieb Stefan Tauner: > > after a short discussion with carldani i have removed the assert stuff and > > do some inferior array checking in selfcheck. > > > > we only have _extern_ > > references to the tables to sizeof does not work. we could add a "getter" > > for the sizes of the array or bring the tables into view to improve this... > > but i dont think it's worth it. > > IMHO assert should be avoided because it is the equivalent of crashing > (no cleanup, no graceful error handling). Assert is OK if memory > corruption or a similar problem has been detected and you just want to > abort as quickly as possible without any cleanup to inimize further damage. > The tests for NULL pointers of chipset_enables etc. make a lot of sense, > but the size checks only make sense if you know the array size from > elsewhere and if you want to check that the array size matches the size > an array walker (loop until ->somemember==NULL) would see. That means > the selfcheck function would have to live in the same file as the array > or you use another global variable which holds sizeof(array). > or making the size accessible out of scope with global functions, yes. do we want that? i think it's ok for now. you did not mention "flashchips". i would not think that there exists a minimal use case for having that one empty, but just to be sure: the application of "|| flashchips[0].vendor == NULL" is ok there? > The code below is internal-only. Please make sure it still compiles with > # make CONFIG_INTERNAL=no > That usually means wrapping in #if CONFIG_INTERNAL == 1 did not think about that sorry, done. > > Not your fault, but I think the \n at the beginning here is really ugly. > It should live in the caller. > […] i have removed the \n at the start of all the print_supported_* functions and rearranged line breaks in print_supported in general a bit. > > - int status; > > + int status; /* OK=0 and NT=1 are defines only. Beware! */ > > > > Do we want an enum instead? i like strong types, but i dont care too much in this case. new patch attached. -- Kind regards/Mit freundlichen Grüßen, Stefan Tauner
>From c582e7a9948a10f3867215f169b76691f4c77a02 Mon Sep 17 00:00:00 2001 From: Stefan Tauner <[email protected]> Date: Wed, 25 May 2011 18:14:03 +0200 Subject: [PATCH] eliminate magic numbers indicating maximum column sizes in print_supported_chipsets and print_supported_boards_helper without this the magic numbers need to be kept in sync with the maximum length of the strings printed in the corresponding column. if not, an overflow and a nasty ' '-storm occur on executing flashrom -L. Signed-off-by: Stefan Tauner <[email protected]> --- flashrom.c | 28 ++++++++++++++++ print.c | 102 ++++++++++++++++++++++++++++++++++++++------------------- programmer.h | 8 ++-- 3 files changed, 100 insertions(+), 38 deletions(-) diff --git a/flashrom.c b/flashrom.c index 46c9ecd..14d5263 100644 --- a/flashrom.c +++ b/flashrom.c @@ -1709,9 +1709,37 @@ int selfcheck(void) msg_gerr("Programmer table miscompilation!\n"); ret = 1; } + /* It would be favorable if we could also check for correct terminaion + * of the follwing arrays, but we don't know their size in here... + * For 'flashchips' we check the first element to be non-null. In the + * other cases there exist use cases where the first element can be + * null. */ + if (flashchips == NULL || flashchips[0].vendor == NULL) { + msg_gerr("Flashchips table miscompilation!\n"); + ret = 1; + } for (flash = flashchips; flash && flash->name; flash++) if (selfcheck_eraseblocks(flash)) ret = 1; + +#if CONFIG_INTERNAL == 1 + if (chipset_enables == NULL) { + msg_gerr("Chipset enables table does not exist!\n"); + ret = 1; + } + if (board_pciid_enables == NULL) { + msg_gerr("Board enables table does not exist!\n"); + ret = 1; + } + if (boards_known == NULL) { + msg_gerr("Known boards table does not exist!\n"); + ret = 1; + } + if (laptops_known == NULL) { + msg_gerr("Known laptops table does not exist!\n"); + ret = 1; + } +#endif // CONFIG_INTERNAL == 1 return ret; } diff --git a/print.c b/print.c index e84a417..fc66706 100644 --- a/print.c +++ b/print.c @@ -77,7 +77,8 @@ static int digits(int n) static void print_supported_chips(void) { int okcol = 0, pos = 0, i, chipcount = 0; - int maxchiplen = 0, maxvendorlen = 0; + int maxvendorlen = strlen("Vendor") + 1; + int maxchiplen = strlen("Device") + 1; const struct flashchip *f; for (f = flashchips; f->name != NULL; f++) { @@ -92,7 +93,7 @@ static void print_supported_chips(void) maxchiplen++; okcol = maxvendorlen + maxchiplen; - printf("\nSupported flash chips (total: %d):\n\n", chipcount); + printf("Supported flash chips (total: %d):\n\n", chipcount); printf("Vendor"); for (i = strlen("Vendor"); i < maxvendorlen; i++) printf(" "); @@ -158,63 +159,94 @@ static void print_supported_chips(void) #if CONFIG_INTERNAL == 1 static void print_supported_chipsets(void) { - int i, j, chipsetcount = 0; + int i, chipsetcount = 0; const struct penable *c = chipset_enables; + int maxvendorlen = strlen("Vendor") + 1; + int maxchipsetlen = strlen("Chipset") + 1; - for (i = 0; c[i].vendor_name != NULL; i++) + for (c = chipset_enables; c->vendor_name != NULL; c++) { chipsetcount++; + maxvendorlen = max(maxvendorlen, strlen(c->vendor_name)); + maxchipsetlen = max(maxchipsetlen, strlen(c->device_name)); + } + maxvendorlen++; + maxchipsetlen++; + + printf("Supported chipsets (total: %d):\n\n", chipsetcount); + + printf("Vendor"); + for (i = strlen("Vendor"); i < maxvendorlen; i++) + printf(" "); - printf("\nSupported chipsets (total: %d):\n\nVendor: " - "Chipset: PCI IDs:\n\n", chipsetcount); + printf("Chipset"); + for (i = strlen("Chipset"); i < maxchipsetlen; i++) + printf(" "); - for (i = 0; c[i].vendor_name != NULL; i++) { - printf("%s", c[i].vendor_name); - for (j = 0; j < 25 - strlen(c[i].vendor_name); j++) + printf("PCI IDs State\n\n"); + + for (c = chipset_enables; c->vendor_name != NULL; c++) { + printf("%s", c->vendor_name); + for (i = 0; i < maxvendorlen - strlen(c->vendor_name); i++) printf(" "); - printf("%s", c[i].device_name); - for (j = 0; j < 25 - strlen(c[i].device_name); j++) + printf("%s", c->device_name); + for (i = 0; i < maxchipsetlen - strlen(c->device_name); i++) printf(" "); - printf("%04x:%04x%s\n", c[i].vendor_id, c[i].device_id, - (c[i].status == OK) ? "" : " (untested)"); + printf("%04x:%04x%s\n", c->vendor_id, c->device_id, + (c->status == OK) ? "" : " (untested)"); } } static void print_supported_boards_helper(const struct board_info *boards, const char *devicetype) { - int i, j, boardcount_good = 0, boardcount_bad = 0; - const struct board_pciid_enable *b = board_pciid_enables; - - for (i = 0; boards[i].vendor != NULL; i++) { - if (boards[i].working) + int i, boardcount_good = 0, boardcount_bad = 0; + const struct board_pciid_enable *e = board_pciid_enables; + const struct board_info *b = boards; + int maxvendorlen = strlen("Vendor") + 1; + int maxboardlen = strlen("Board") + 1; + + for (b = boards; b->vendor != NULL; b++) { + maxvendorlen = max(maxvendorlen, strlen(b->vendor)); + maxboardlen = max(maxboardlen, strlen(b->name)); + if (b->working) boardcount_good++; else boardcount_bad++; } + maxvendorlen++; + maxboardlen++; + + printf("Known %s (good: %d, bad: %d):\n\n", + devicetype, boardcount_good, boardcount_bad); - printf("\nKnown %s (good: %d, bad: %d):" - "\n\nVendor: Board: " - "Status: Required option:" - "\n\n", devicetype, boardcount_good, boardcount_bad); + printf("Vendor"); + for (i = strlen("Vendor"); i < maxvendorlen; i++) + printf(" "); - for (i = 0; boards[i].vendor != NULL; i++) { - printf("%s", boards[i].vendor); - for (j = 0; j < 25 - strlen(boards[i].vendor); j++) + printf("Board"); + for (i = strlen("Board"); i < maxboardlen; i++) + printf(" "); + + printf("Status Required option\n\n"); + + for (b = boards; b->vendor != NULL; b++) { + printf("%s", b->vendor); + for (i = 0; i < maxvendorlen - strlen(b->vendor); i++) printf(" "); - printf("%s", boards[i].name); - for (j = 0; j < 28 - strlen(boards[i].name); j++) + printf("%s", b->name); + for (i = 0; i < maxboardlen - strlen(b->name); i++) printf(" "); - printf((boards[i].working) ? "OK " : "BAD "); + printf((b->working) ? "OK " : "BAD "); - for (j = 0; b[j].vendor_name != NULL; j++) { - if (strcmp(b[j].vendor_name, boards[i].vendor) - || strcmp(b[j].board_name, boards[i].name)) + for (e = board_pciid_enables; e->vendor_name != NULL; e++) { + if (strcmp(e->vendor_name, b->vendor) + || strcmp(e->board_name, b->name)) continue; - if (b[j].lb_vendor == NULL) + if (e->lb_vendor == NULL) printf("(autodetected)"); else - printf("-m %s:%s", b[j].lb_vendor, - b[j].lb_part); + printf("-m %s:%s", e->lb_vendor, + e->lb_part); } printf("\n"); } @@ -228,10 +260,12 @@ void print_supported(void) printf("\nSupported programmers:\n"); list_programmers_linebreak(0, 80, 0); #if CONFIG_INTERNAL == 1 - printf("\nSupported devices for the %s programmer:\n", + printf("\nSupported devices for the %s programmer:\n\n", programmer_table[PROGRAMMER_INTERNAL].name); print_supported_chipsets(); + printf("\n"); print_supported_boards_helper(boards_known, "boards"); + printf("\n"); print_supported_boards_helper(laptops_known, "laptops"); #endif #if CONFIG_DUMMY == 1 diff --git a/programmer.h b/programmer.h index b68aa88..e1147f1 100644 --- a/programmer.h +++ b/programmer.h @@ -145,7 +145,7 @@ struct bitbang_spi_master { struct penable { uint16_t vendor_id; uint16_t device_id; - int status; + int status; /* OK=0 and NT=1 are defines only. Beware! */ const char *vendor_name; const char *device_name; int (*doit) (struct pci_dev *dev, const char *name); @@ -174,10 +174,10 @@ struct board_pciid_enable { uint16_t second_card_vendor; uint16_t second_card_device; - /* Pattern to match DMI entries */ + /* Pattern to match DMI entries. May be NULL. */ const char *dmi_pattern; - /* The vendor / part name from the coreboot table. */ + /* The vendor / part name from the coreboot table. May be NULL. */ const char *lb_vendor; const char *lb_part; @@ -188,7 +188,7 @@ struct board_pciid_enable { int max_rom_decode_parallel; int status; - int (*enable) (void); + int (*enable) (void); /* May be NULL. */ }; extern const struct board_pciid_enable board_pciid_enables[]; -- 1.7.1
_______________________________________________ flashrom mailing list [email protected] http://www.flashrom.org/mailman/listinfo/flashrom
