New subject: [PATCH] eliminate magic numbers indicating maximum column sizes in print_supported_chipsets and print_supported_boards_helper

24 May 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 Taunerstefan.tauner@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/



    

Re: [flashrom] [PATCH] eliminate magic numbers indicating maximum column sizes in print_supported_chipsets and print_supported_boards_helper

Signed-off-by: Stefan Taunerstefan.tauner@student.tuwien.ac.at