Am 18.06.2011 22:53 schrieb Stefan Tauner:
besides adding output for the voltage ranges, this patch also changes various aspects of the -L output:
- sizes are right aligned now with a fixed length of 5
- test results are always shown in the same column ("PR" and " R" instead of "PR" and "R ")
- get rid of POS_PRINT and digits
- wideness reductions on the whole with a remarkable detail: vendor names are split on '/' and spread over mutliple lines while all other columns are printed on the first line.
Signed-off-by: Stefan Tauner stefan.tauner@student.tuwien.ac.at
Sounds good, I'll review in detail later. A few comments, though.
print.c | 149 +++++++++++++++++++++++++++++++++++++++++++-------------------- 1 files changed, 102 insertions(+), 47 deletions(-)
diff --git a/print.c b/print.c index 80316cb..b110ff9 100644 --- a/print.c +++ b/print.c @@ -59,27 +59,16 @@ char *flashbuses_to_text(enum chipbustype bustype) return ret; }
-#define POS_PRINT(x) do { pos += strlen(x); msg_ginfo(x); } while (0)
-static int digits(int n) -{
- int i;
- if (!n)
return 1;- for (i = 0; n; ++i)
n /= 10;- return i;
-}
static void print_supported_chips(void) {
- int okcol = 0, pos = 0, i, chipcount = 0;
- int i, chipcount = 0; int maxvendorlen = strlen("Vendor") + 1; int maxchiplen = strlen("Device") + 1;
- int maxtypelen = strlen("Type") + 1; const struct flashchip *f;
- const char *delim ="/";
- char *tmp_ven_str;
- int tmp_ven_len;
Ugly variable names.
char *tmp_bus_str;
for (f = flashchips; f->name != NULL; f++) { @@ -87,12 +76,22 @@ static void print_supported_chips(void) if (!strncmp(f->name, "unknown", 7)) continue; chipcount++;
maxvendorlen = max(maxvendorlen, strlen(f->vendor));
tmp_ven_str = (char *)f->vendor;do {tmp_ven_len = strcspn(tmp_ven_str, delim);maxvendorlen = max(maxvendorlen, tmp_ven_len);tmp_ven_str += tmp_ven_len;
You need to add +1 to the expression above. Explanation follows.
} while (tmp_ven_len != 0);
Are you sure this loop does what it should? Consider maxvendorlen=0, vendor="a/bc". The first iteration will have tmp_ven_len=1, maxvendorlen=1, then tmp_ven_str="/bc". The next iteration will have tmp_ven_len=0, maxvendorlen=1, tmp_ven_str="/bc" and then the loop will stop with an incorrect value of maxvendorlen.
- maxchiplen = max(maxchiplen, strlen(f->name));
tmp_bus_str = flashbuses_to_text(f->bustype);maxtypelen = max(maxtypelen,strlen(flashbuses_to_text(f->bustype)));
Oooh... that one is fun. Do we really want that, or do we just want to assume that maximum string length of bustype is the length of all bustypes at once?
free(tmp_bus_str);
- maxvendorlen++;
- maxchiplen++;
- okcol = maxvendorlen + maxchiplen;
- maxvendorlen += 1;
- maxchiplen += 1;
- maxtypelen += 1;
Is there any reason you're not using ++?
msg_ginfo("Supported flash chips (total: %d):\n\n", chipcount); msg_ginfo("Vendor");
The parts below were not reviewed yet, I need to run flashrom with that patch to check if the output really is OK.
@@ -102,10 +101,21 @@ static void print_supported_chips(void) for (i = strlen("Device"); i < maxchiplen; i++) msg_ginfo(" ");
- msg_ginfo("Tested Known Size/kB: Type:\n");
- for (i = 0; i < okcol; i++)
- msg_ginfo("Test Known Size ");
- msg_ginfo("Type");
- for (i = strlen("Type"); i < maxtypelen; i++)
msg_ginfo(" ");- msg_gdbg("Voltage");
- msg_ginfo("\n");
- for (i = 0; i < maxvendorlen + maxchiplen; i++)
msg_ginfo(" ");- msg_ginfo("OK Broken [kB]");
- for (i = 0; i < maxtypelen; i++) msg_ginfo(" ");
- msg_ginfo("OK Broken\n\n");
msg_gdbg("range [V]");
msg_ginfo("\n\n"); msg_ginfo("(P = PROBE, R = READ, E = ERASE, W = WRITE)\n\n");
for (f = flashchips; f->name != NULL; f++) {
@@ -113,50 +123,95 @@ static void print_supported_chips(void) if (!strncmp(f->name, "unknown", 7)) continue;
msg_ginfo("%s", f->vendor);for (i = strlen(f->vendor); i < maxvendorlen; i++)
/* support for multiline vendor names:* - make a copy of the original vendor name* - use strok to put the next token in tmp_ven_str repeatedly* - keep track of the last token's length for ' '-padding*/tmp_ven_str = malloc(strlen(f->vendor) + 1);if (tmp_ven_str == NULL) {msg_gerr("Out of memory!\n");exit(1);}strcpy(tmp_ven_str, f->vendor);tmp_ven_str = strtok(tmp_ven_str, delim);tmp_ven_len = strlen(tmp_ven_str);msg_ginfo("%s", tmp_ven_str);tmp_ven_str = strtok(NULL, delim);if (tmp_ven_str != NULL) {msg_ginfo("%s", delim);tmp_ven_len++;}for (i = tmp_ven_len; i < maxvendorlen; i++) msg_ginfo(" ");- msg_ginfo("%s", f->name); for (i = strlen(f->name); i < maxchiplen; i++) msg_ginfo(" ");
pos = maxvendorlen + maxchiplen;POS_PRINT("P ");
msg_ginfo("P");elsemsg_ginfo(" "); if ((f->tested & TEST_OK_READ))
POS_PRINT("R ");
msg_ginfo("R");elsemsg_ginfo(" "); if ((f->tested & TEST_OK_ERASE))
POS_PRINT("E ");
msg_ginfo("E");elsemsg_ginfo(" "); if ((f->tested & TEST_OK_WRITE))
POS_PRINT("W ");}while (pos < okcol + 9) {msg_ginfo(" ");pos++;}
msg_ginfo("W ");elsemsg_ginfo(" ");} elsemsg_ginfo(" ");- if ((f->tested & TEST_BAD_MASK)) { if ((f->tested & TEST_BAD_PROBE))
POS_PRINT("P ");
msg_ginfo("P");elsemsg_ginfo(" "); if ((f->tested & TEST_BAD_READ))
POS_PRINT("R ");
msg_ginfo("R");elsemsg_ginfo(" "); if ((f->tested & TEST_BAD_ERASE))
POS_PRINT("E ");
msg_ginfo("E");elsemsg_ginfo(" "); if ((f->tested & TEST_BAD_WRITE))
POS_PRINT("W ");}
msg_ginfo("W ");elsemsg_ginfo(" ");} elsemsg_ginfo(" ");
while (pos < okcol + 18) {msg_ginfo(" ");pos++;}msg_ginfo("%d", f->total_size);for (i = 0; i < 10 - digits(f->total_size); i++)msg_ginfo(" ");
msg_ginfo("%5d ", f->total_size);tmp_bus_str = flashbuses_to_text(f->bustype); msg_ginfo("%s", tmp_bus_str);
for (i = strlen(tmp_bus_str);i < maxtypelen;i++)msg_ginfo(" ");free(tmp_bus_str);
if (f->voltage.min == 0 && f->voltage.max == 0)msg_gdbg("no info");elsemsg_gdbg("%0.02f;%0.02f",f->voltage.min/(double)1000,f->voltage.max/(double)1000);while (tmp_ven_str != NULL) {msg_ginfo("\n%s", tmp_ven_str);tmp_ven_str = strtok(NULL, delim);}msg_ginfo("\n"); }
}
Regards, Carl-Daniel