David Hendricks has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/37407 )
Change subject: print.c: Fix alignment in print_supported_boards_helper() ......................................................................
print.c: Fix alignment in print_supported_boards_helper()
Commit 61e16e5 eliminated some duplicate logic for printing test status, but in doing so introduced a regression where some entries in `flashrom -L` output became unaligned.
Up until then, it was assumed that the status field will always be 8-wide, including space for padding. This patch fixes alignment when using test_state_to_text() by making the padding consistent with other fields where a for-loop is used to pad the field up to a maximum width.
Change-Id: Ie0a0b45c6466d14447aca443c2697e2048c6ef14 Signed-off-by: David Hendricks david.hendricks@gmail.com --- M flash.h M print.c 2 files changed, 29 insertions(+), 14 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/07/37407/1
diff --git a/flash.h b/flash.h index 1a9bd9f..e7b1c89 100644 --- a/flash.h +++ b/flash.h @@ -145,10 +145,11 @@
enum test_state { OK = 0, - NT = 1, /* Not tested */ - BAD, /* Known to not work */ - DEP, /* Support depends on configuration (e.g. Intel flash descriptor) */ - NA, /* Not applicable (e.g. write support on ROM chips) */ + NT = 1, /* Not tested */ + BAD, /* Known to not work */ + DEP, /* Support depends on configuration (e.g. Intel flash descriptor) */ + NA, /* Not applicable (e.g. write support on ROM chips) */ + TEST_STATE_END, /* Not a test state, just an indicator of how many are defined. */ };
#define TEST_UNTESTED (struct tested){ .probe = NT, .read = NT, .erase = NT, .write = NT } diff --git a/print.c b/print.c index 6a7ff5d..e5be6b6 100644 --- a/print.c +++ b/print.c @@ -35,6 +35,20 @@ } }
+static size_t max_test_state_len(void) +{ + int i; + size_t max_len; + + for (i = max_len = 0; i < TEST_STATE_END; i++) { + size_t len = strlen(test_state_to_text(i)); + if (len > max_len) + max_len = len; + } + + return max_len; +} + static int print_supported_chips(void) { const char *delim = "/"; @@ -388,8 +402,12 @@ for (i = strlen("Board"); i < maxboardlen; i++) msg_ginfo(" ");
- msg_ginfo("Status Required value for\n"); - for (i = 0; i < maxvendorlen + maxboardlen + strlen("Status "); i++) + msg_ginfo("Status"); + for (i = strlen("Status"); i < max_test_state_len(); i++) + msg_ginfo(" "); + + msg_ginfo("Required value for\n"); + for (i = 0; i < maxvendorlen + maxboardlen + max_test_state_len(); i++) msg_ginfo(" "); msg_ginfo("-p internal:mainboard=\n");
@@ -401,14 +419,10 @@ for (i = 0; i < maxboardlen - strlen(b->name); i++) msg_ginfo(" ");
- switch (b->working) { - case OK: msg_ginfo("OK "); break; - case NT: msg_ginfo("NT "); break; - case DEP: msg_ginfo("DEP "); break; - case NA: msg_ginfo("N/A "); break; - case BAD: - default: msg_ginfo("BAD "); break; - } + const char *test_state = test_state_to_text(b->working); + msg_ginfo("%s", test_state); + for (i = 0; i < max_test_state_len() - strlen(test_state); i++) + msg_ginfo(" ");
for (e = board_matches; e->vendor_name != NULL; e++) { if (strcmp(e->vendor_name, b->vendor)
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/37407 )
Change subject: print.c: Fix alignment in print_supported_boards_helper() ......................................................................
Patch Set 1: Code-Review+2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/37407 )
Change subject: print.c: Fix alignment in print_supported_boards_helper() ......................................................................
Patch Set 1:
IMHO, this is unnecessarily complicated. Also, I believe the short-forms are better to display on tables.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/37407 )
Change subject: print.c: Fix alignment in print_supported_boards_helper() ......................................................................
Patch Set 1:
Sorry, I really don't want any quarrel. But if we add 15 lines to dedup 5, I have to ask, what does this fix? Somebody put much care into this table in the past. And now we want to destroy that just for the sake of doing something?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/37407 )
Change subject: print.c: Fix alignment in print_supported_boards_helper() ......................................................................
Patch Set 1: Code-Review-1
(1 comment)
Patch Set 1:
Sorry, I really don't want any quarrel. But if we add 15 lines to dedup 5, I have to ask, what does this fix? Somebody put much care into this table in the past. And now we want to destroy that just for the sake of doing something?
I don't see why this would be an improvement. If the short forms are not obvious, maybe print their meaning as done with PREW?
(P = PROBE, R = READ, E = ERASE, W = WRITE, - = N/A)
https://review.coreboot.org/c/flashrom/+/37407/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/flashrom/+/37407/1//COMMIT_MSG@9 PS1, Line 9: 61e16e5 Wasn't this reverted in commit 5d068dd though?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/37407 )
Change subject: print.c: Fix alignment in print_supported_boards_helper() ......................................................................
Patch Set 1:
Patch Set 1: Code-Review-1
(1 comment)
Patch Set 1:
Sorry, I really don't want any quarrel. But if we add 15 lines to dedup 5, I have to ask, what does this fix? Somebody put much care into this table in the past. And now we want to destroy that just for the sake of doing something?
I don't see why this would be an improvement. If the short forms are not obvious, maybe print their meaning as done with PREW?
(P = PROBE, R = READ, E = ERASE, W = WRITE, - = N/A)
Oh, nice. Looks like flashrom -L is broken... cf. CB:39826
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/37407 )
Change subject: print.c: Fix alignment in print_supported_boards_helper() ......................................................................
Patch Set 1: -Code-Review
David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/37407 )
Change subject: print.c: Fix alignment in print_supported_boards_helper() ......................................................................
Patch Set 1:
Patch Set 1: Code-Review-1
(1 comment)
Patch Set 1:
Sorry, I really don't want any quarrel. But if we add 15 lines to dedup 5, I have to ask, what does this fix? Somebody put much care into this table in the past. And now we want to destroy that just for the sake of doing something?
I don't see why this would be an improvement. If the short forms are not obvious, maybe print their meaning as done with PREW?
(P = PROBE, R = READ, E = ERASE, W = WRITE, - = N/A)
Sure, we can try to use the short forms and eliminate the long forms used in test_state_to_text() if that is preferred.
Just as a reminder since it's been a while: The purpose of this patch was to calculate the width of each fields in a consistent manner since the existing logic proved to be fragile.
-
Angel Pons (Code Review) -
David Hendricks (Code Review)
-
Edward O'Callaghan (Code Review)
-
Nico Huber (Code Review)