[flashrom] [PATCH] eliminate magic numbers indicating maximum column sizes in print_supported_chipsets and print_supported_boards_helper
Carl-Daniel Hailfinger
c-d.hailfinger.devel.2006 at gmx.net
Tue May 24 09:01:13 CEST 2011
Am 24.05.2011 02:21 schrieb Stefan Tauner:
> 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 occurs
> on executing flashrom -L.
>
> Signed-off-by: Stefan Tauner<stefan.tauner at student.tuwien.ac.at>
> ---
>
> after a short discussion with carldani i have removed the assert stuff and
> do some inferior array checking in selfcheck.
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.
> 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.
>
> flashrom.c | 22 +++++++++++++
> print.c | 96 ++++++++++++++++++++++++++++++++++++++-------------------
> programmer.h | 8 ++--
> 3 files changed, 90 insertions(+), 36 deletions(-)
>
> diff --git a/flashrom.c b/flashrom.c
> index 46c9ecd..821d745 100644
> --- a/flashrom.c
> +++ b/flashrom.c
> @@ -1709,6 +1709,28 @@ 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... */
> + if (flashchips == NULL || flashchips[0].vendor == NULL) {
> + msg_gerr("Flashchips table miscompilation!\n");
> + ret = 1;
> + }
>
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
> + if (chipset_enables == NULL || chipset_enables[0].vendor_name == NULL) {
> + msg_gerr("Chipset enables table miscompilation!\n");
> + ret = 1;
> + }
>
chipset_enables[0].vendor_name==NULL is a valid case if someone wants to
run a minimal flashrom on a machine which does not need a chipset enable.
> + if (board_pciid_enables == NULL || board_pciid_enables[0].vendor_name == NULL) {
> + msg_gerr("Board enables table miscompilation!\n");
> + ret = 1;
> + }
>
board_pciid_enables[0].vendor_name == NULL is a valid case for minimal
flashrom on a board which does not need a board enable.
> + if (boards_known == NULL || boards_known[0].vendor == NULL) {
> + msg_gerr("Known boards table miscompilation!\n");
> + ret = 1;
> + }
>
Same here if someone does not care about --list-supported output at all
(e.g. in embedded usage).
> + if (laptops_known == NULL || laptops_known[0].vendor == NULL) {
> + msg_gerr("Known laptops table miscompilation!\n");
> + ret = 1;
> + }
>
Dito.
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).
Put #endif here.
> for (flash = flashchips; flash&& flash->name; flash++)
> if (selfcheck_eraseblocks(flash))
> ret = 1;
> diff --git a/print.c b/print.c
> index e84a417..b0f8247 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++) {
> @@ -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("\nSupported chipsets (total: %d):\n\n", chipsetcount);
>
Not your fault, but I think the \n at the beginning here is really ugly.
It should live in the caller.
> +
> + printf("Vendor");
> + for (i = strlen("Vendor"); i< maxvendorlen; i++)
> + printf(" ");
> +
> + printf("Chipset");
> + for (i = strlen("Chipset"); i< maxchipsetlen; i++)
> + printf(" ");
>
> - printf("\nSupported chipsets (total: %d):\n\nVendor: "
> - "Chipset: PCI IDs:\n\n", chipsetcount);
> + printf("PCI IDs State\n\n");
>
> - 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++)
> + 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("\nKnown %s (good: %d, bad: %d):\n\n",
> + devicetype, boardcount_good, boardcount_bad);
>
Not your fault, but I think the \n at the beginning here is really ugly.
It should live in the caller.
> +
> + printf("Vendor");
> + for (i = strlen("Vendor"); i< maxvendorlen; i++)
> + printf(" ");
> +
> + printf("Board");
> + for (i = strlen("Board"); i< maxboardlen; i++)
> + printf(" ");
>
> - printf("\nKnown %s (good: %d, bad: %d):"
> - "\n\nVendor: Board: "
> - "Status: Required option:"
> - "\n\n", devicetype, boardcount_good, boardcount_bad);
> + printf("Status Required option\n\n");
>
> - for (i = 0; boards[i].vendor != NULL; i++) {
> - printf("%s", boards[i].vendor);
> - for (j = 0; j< 25 - strlen(boards[i].vendor); j++)
> + 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");
> }
> 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! */
>
Do we want an enum instead?
> 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[];
>
Looks good otherwise.
Regards,
Carl-Daniel
--
http://www.hailfinger.org/
More information about the flashrom
mailing list