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.tau...@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;
> if ((f->tested & TEST_OK_MASK)) {
> if ((f->tested & TEST_OK_PROBE))
> - POS_PRINT("P ");
> + msg_ginfo("P");
> + else
> + msg_ginfo(" ");
> if ((f->tested & TEST_OK_READ))
> - POS_PRINT("R ");
> + msg_ginfo("R");
> + else
> + msg_ginfo(" ");
> if ((f->tested & TEST_OK_ERASE))
> - POS_PRINT("E ");
> + msg_ginfo("E");
> + else
> + msg_ginfo(" ");
> if ((f->tested & TEST_OK_WRITE))
> - POS_PRINT("W ");
> - }
> - while (pos < okcol + 9) {
> - msg_ginfo(" ");
> - pos++;
> - }
> + msg_ginfo("W ");
> + else
> + msg_ginfo(" ");
> + } else
> + msg_ginfo(" ");
> +
> if ((f->tested & TEST_BAD_MASK)) {
> if ((f->tested & TEST_BAD_PROBE))
> - POS_PRINT("P ");
> + msg_ginfo("P");
> + else
> + msg_ginfo(" ");
> if ((f->tested & TEST_BAD_READ))
> - POS_PRINT("R ");
> + msg_ginfo("R");
> + else
> + msg_ginfo(" ");
> if ((f->tested & TEST_BAD_ERASE))
> - POS_PRINT("E ");
> + msg_ginfo("E");
> + else
> + msg_ginfo(" ");
> if ((f->tested & TEST_BAD_WRITE))
> - POS_PRINT("W ");
> - }
> + msg_ginfo("W ");
> + else
> + msg_ginfo(" ");
> + } else
> + msg_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");
> + else
> + msg_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
--
http://www.hailfinger.org/
_______________________________________________
flashrom mailing list
flashrom@flashrom.org
http://www.flashrom.org/mailman/listinfo/flashrom
