Re: [flashrom] [PATCH 3/4] reformat -L output and add printing of chip voltage ranges to print.c
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
On Sat, 25 Jun 2011 19:27:01 +0200 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
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.
tmpven and tmpvenlen? else i wanna hear suggestions :)
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.
fixed and annotated with: /* skip to the char following the first '/' */ thanks!
- 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?
using the combined length of all of them together is safe but would lead to a very wide column. what's your problem with this version (besides the wrong usage of flashbuses_to_text instead of tmp_bus_str)? it looks at the length of all buses combined in each chip. why would we want to print more than that?
free(tmp_bus_str);
- maxvendorlen++;
- maxchiplen++;
- okcol = maxvendorlen + maxchiplen;
- maxvendorlen += 1;
- maxchiplen += 1;
- maxtypelen += 1;
Is there any reason you're not using ++?
yes, because i have used += 2 in an earlier version ;)
[…]
On Sat, 25 Jun 2011 20:30:18 +0200 Stefan Tauner stefan.tauner@student.tuwien.ac.at wrote:
On Sat, 25 Jun 2011 19:27:01 +0200 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
Am 18.06.2011 22:53 schrieb Stefan Tauner:
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.
fixed and annotated with: /* skip to the char following the first '/' */ thanks!
actually it is not as easy that. with your change added there is another "minor" problem :) in the last token (or if there is only one) tmp_ven_str will point at the memory address after '\0' resulting in strlen searching where we surely don't want it to in the next iteration.
so basically the problem is that we can't distinguish between "token found" and "end of string found".
my current solution is to check for that explicitly. checking for \0 and break;-ing is also possible. we could use while(1) then if i see that right....
do { tmp_ven_len = strcspn(tmp_ven_str, delim); maxvendorlen = max(maxvendorlen, tmp_ven_len); /* skip to the char following the first '/' */ tmp_ven_str += tmp_ven_len; if (tmp_ven_str[0] == delim[0]) tmp_ven_str++; } while (tmp_ven_len != 0);
the var[0] could be changed to *var, but i think it is clearer so.
-
Carl-Daniel Hailfinger -
Stefan Tauner