Am 02.01.2012 11:29 schrieb Stefan Tauner:
On Mon, 02 Jan 2012 03:48:40 +0100 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
--mainboard is a relic from a time before external programmers and makes the CLI inconsistent. Use a programmer parameter instead and free up the short option -m.
A nice side effect is that there is one less corner case we have to care about in the logging patch.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
the following hunk should get its own note in the changelog if it is merged imho.
Index: flashrom-kill_cli_mainboard_parameter/cbtable.c
--- flashrom-kill_cli_mainboard_parameter/cbtable.c (Revision 1482) +++ flashrom-kill_cli_mainboard_parameter/cbtable.c (Arbeitskopie) @@ -33,17 +33,19 @@ char *lb_part = NULL, *lb_vendor = NULL; int partvendor_from_cbtable = 0;
-void lb_vendor_dev_from_string(char *boardstring) +void lb_vendor_dev_from_string(const char *boardstring) {
- /* strtok may modify the original string. */
- char *tempstr = strdup(boardstring); char *tempstr2 = NULL;
- strtok(boardstring, ":");
- strtok(tempstr, ":"); tempstr2 = strtok(NULL, ":"); if (tempstr2) {
lb_vendor = boardstring;
lb_vendor = tempstr;
lb_part = boardstring;
lb_part = tempstr;hm... we want something like free(tempstr); due to the strdup call but since we change the pointer in the process we do no longer know where the string started. and of course we would still want the extracted tokens to remain valid, so that would require another strdup... oh my.
hm but if the boardstring contains the wanted data only, the current version might be good enough. if we feed it with the return value of extract_programmer_param only (as we do atm) this is somewhat ok. not very obvious but at least not too leaky :)
and if (tempstr == NULL) is not handled (because empty arguments are checked for in the caller?). OTOH it is not really necessary because both strings are set to NULL then, but it is also not obvious that this is wanted.
I tried to get the code right this time, but now it leaks lb_part and lb_vendor allocations (consistent with other places setting lb_part and lb_vendor) and we'd have to free them in some internal_shutdown handler. That said, such one-time leaks are all over the place and should be fixed once programmer_init()/programmer_shutdown() can be called multiple times pairwise in a row.
please add a function comment explaining that stuff.
Sure.
aaaand... please use grep -- -m yourself, there are other bits referring to the old syntax.
Sorry, fixed this time.
the whole cb string handling is a bit messy.
Understatement of the week.
in internal_init we call extract_programmer_param("mainboard") to get the user input. then call lb_vendor_dev_from_string on the result, which internally sets a global variable. later in the same internal_init function we call board_flash_enable with the global variables as arguments.
the only other user is show_id in layout.c
show_id() even has a comment which says it should be moved to cbtable.c.
if we work around that and make lb_vendor_dev_from_string return the two strings, we could make lb_part and lb_vendor static.
id suggest splitting the patch and fix the lb_vendor_dev_from_string hunk together with the other cb table stuff. the -m removals are hard enough apparently to justify a patch on its own ;)
The code calling lb_vendor_dev_from_string() changed too much and I don't really trust myself to remember fixing the bug if I don't do it now. Hopefully it is OK this time.
New patch.
--mainboard is a relic from a time before external programmers and makes the CLI inconsistent. Use a programmer parameter instead and free up the short option -m.
NOTE: The --list-supported-wiki output changed to use -p internal:mainboard= instead of -m The --list-supported output changed the heading of the mainboard list from
Vendor Board Status Required option
to Vendor Board Status Required option for -p internal:mainboard=
I welcome comments on the --list-supported output change.
Fix lb_vendor_dev_from_string() not to write to the supplied string.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: flashrom-kill_cli_mainboard_parameter/cli_classic.c =================================================================== --- flashrom-kill_cli_mainboard_parameter/cli_classic.c (Revision 1482) +++ flashrom-kill_cli_mainboard_parameter/cli_classic.c (Arbeitskopie) @@ -106,7 +106,7 @@ "-z|" #endif "-E|-r <file>|-w <file>|-v <file>]\n" - " [-c <chipname>] [-m [<vendor>:]<part>] [-l <file>]\n" + " [-c <chipname>] [-l <file>]\n" " [-i <image>] [-p <programmername>[:<parameters>]]\n\n");
printf("Please note that the command line interface for flashrom has " @@ -128,11 +128,6 @@ " -V | --verbose more verbose output\n" " -c | --chip <chipname> probe only for specified " "flash chip\n" -#if CONFIG_INTERNAL == 1 - /* FIXME: --mainboard should be a programmer parameter */ - " -m | --mainboard <[vendor:]part> override mainboard " - "detection\n" -#endif " -f | --force force specific operations " "(see man page)\n" " -n | --noverify don't auto-verify\n" @@ -190,7 +185,6 @@ {"verify", 1, NULL, 'v'}, {"noverify", 0, NULL, 'n'}, {"chip", 1, NULL, 'c'}, - {"mainboard", 1, NULL, 'm'}, {"verbose", 0, NULL, 'V'}, {"force", 0, NULL, 'f'}, {"layout", 1, NULL, 'l'}, @@ -275,17 +269,6 @@ } erase_it = 1; break; - case 'm': -#if CONFIG_INTERNAL == 1 - tempstr = strdup(optarg); - lb_vendor_dev_from_string(tempstr); -#else - fprintf(stderr, "Error: Internal programmer support " - "was not compiled in and --mainboard only\n" - "applies to the internal programmer. Aborting.\n"); - cli_classic_abort_usage(); -#endif - break; case 'f': force = 1; break; @@ -426,14 +409,6 @@ if (prog == PROGRAMMER_INVALID) prog = default_programmer;
-#if CONFIG_INTERNAL == 1 - if ((prog != PROGRAMMER_INTERNAL) && (lb_part || lb_vendor)) { - fprintf(stderr, "Error: --mainboard requires the internal " - "programmer. Aborting.\n"); - cli_classic_abort_usage(); - } -#endif - /* FIXME: Delay calibration should happen in programmer code. */ myusec_calibrate_delay();
Index: flashrom-kill_cli_mainboard_parameter/internal.c =================================================================== --- flashrom-kill_cli_mainboard_parameter/internal.c (Revision 1482) +++ flashrom-kill_cli_mainboard_parameter/internal.c (Arbeitskopie) @@ -213,6 +213,16 @@ } free(arg);
+ arg = extract_programmer_param("mainboard"); + if (arg && strlen(arg)) { + lb_vendor_dev_from_string(arg); + } else if (arg && !strlen(arg)) { + msg_perr("Missing argument for mainboard.\n"); + free(arg); + return 1; + } + free(arg); + get_io_perms(); if (register_shutdown(internal_shutdown, NULL)) return 1; Index: flashrom-kill_cli_mainboard_parameter/layout.c =================================================================== --- flashrom-kill_cli_mainboard_parameter/layout.c (Revision 1482) +++ flashrom-kill_cli_mainboard_parameter/layout.c (Arbeitskopie) @@ -106,11 +106,11 @@
/* * If lb_vendor is not set, the coreboot table was - * not found. Nor was -m VENDOR:PART specified. + * not found. Nor was -p internal:mainboard=VENDOR:PART specified. */ if (!lb_vendor || !lb_part) { - msg_pinfo("Note: If the following flash access fails, " - "try -m <vendor>:<mainboard>.\n"); + msg_pinfo("Note: If the following flash access fails, try " + "-p internal:mainboard=<vendor>:<mainboard>.\n"); return 0; }
@@ -126,14 +126,17 @@ "seem to fit to this machine - forcing it.\n"); } else { msg_pinfo("ERROR: Your firmware image (%s:%s) does not " - "appear to\n be correct for the detected " - "mainboard (%s:%s)\n\nOverride with -p internal:" - "boardmismatch=force if you are absolutely sure " - "that\nyou are using a correct " - "image for this mainboard or override\nthe detected " - "values with --mainboard <vendor>:<mainboard>.\n\n", - mainboard_vendor, mainboard_part, lb_vendor, - lb_part); + "appear to\n" + " be correct for the detected " + "mainboard (%s:%s)\n\n" + "Override with -p internal:boardmismatch=" + "force if you are absolutely sure that\n" + "you are using a correct image for this " + "mainboard or override\n" + "the detected values with -p internal:" + "mainboard=<vendor>:<mainboard>.\n\n", + mainboard_vendor, mainboard_part, lb_vendor, + lb_part); exit(1); } } Index: flashrom-kill_cli_mainboard_parameter/print_wiki.c =================================================================== --- flashrom-kill_cli_mainboard_parameter/print_wiki.c (Revision 1482) +++ flashrom-kill_cli_mainboard_parameter/print_wiki.c (Arbeitskopie) @@ -167,7 +167,7 @@ boards[i].url ? boards[i].url : "", boards[i].name, boards[i].url ? "]" : "", - b[k].lb_vendor ? "-m " : "—", + b[k].lb_vendor ? "-p internal:mainboard= " : "—", b[k].lb_vendor ? b[k].lb_vendor : "", b[k].lb_vendor ? ":" : "", b[k].lb_vendor ? b[k].lb_part : "", Index: flashrom-kill_cli_mainboard_parameter/cbtable.c =================================================================== --- flashrom-kill_cli_mainboard_parameter/cbtable.c (Revision 1482) +++ flashrom-kill_cli_mainboard_parameter/cbtable.c (Arbeitskopie) @@ -33,18 +33,21 @@ char *lb_part = NULL, *lb_vendor = NULL; int partvendor_from_cbtable = 0;
-void lb_vendor_dev_from_string(char *boardstring) +void lb_vendor_dev_from_string(const char *boardstring) { + /* strtok may modify the original string. */ + char *tempstr = strdup(boardstring); char *tempstr2 = NULL; - strtok(boardstring, ":"); + strtok(tempstr, ":"); tempstr2 = strtok(NULL, ":"); if (tempstr2) { - lb_vendor = boardstring; - lb_part = tempstr2; + lb_vendor = strdup(tempstr); + lb_part = strdup(tempstr2); } else { lb_vendor = NULL; - lb_part = boardstring; + lb_part = strdup(tempstr); } + free(tempstr); }
static unsigned long compute_checksum(void *addr, unsigned long length) Index: flashrom-kill_cli_mainboard_parameter/flashrom.8 =================================================================== --- flashrom-kill_cli_mainboard_parameter/flashrom.8 (Revision 1482) +++ flashrom-kill_cli_mainboard_parameter/flashrom.8 (Arbeitskopie) @@ -5,7 +5,7 @@ .B flashrom \fR[\fB-n\fR] [\fB-V\fR] [\fB-f\fR] [\fB-h\fR|\fB-R\fR|\ \fB-L\fR|\fB-z\fR|\fB-E\fR|\fB-r\fR <file>|\fB-w\fR <file>|\ \fB-v\fR <file>] - [\fB-c\fR <chipname>] [\fB-m\fR [<vendor>:]<board>] \ + [\fB-c\fR <chipname>] \ [\fB-l\fR <file>] [\fB-i\fR <image>] [\fB-p\fR <programmername>[:<parameters>]] .SH DESCRIPTION @@ -88,19 +88,6 @@ without the vendor name as parameter. Please note that the chip name is case sensitive. .TP -.B "-m, --mainboard" [<vendor>:]<board> -Override mainboard settings. -.sp -flashrom reads the coreboot table to determine the current mainboard. If no -coreboot table could be read or if you want to override these values, you can -specify -m, e.g.: -.sp -.B " flashrom --mainboard AGAMI:ARUMA -w agami_aruma.rom" -.sp -See the 'Known boards' or 'Known laptops' section in the output -of 'flashrom -L' for a list of boards which require the specification of -the board name, if no coreboot table is found. -.TP .B "-f, --force" Force one or more of the following actions: .sp @@ -245,11 +232,16 @@ running coreboot, the mainboard type is determined from the coreboot table. Otherwise, the mainboard is detected by examining the onboard PCI devices and possibly DMI info. If PCI and DMI do not contain information to uniquely -identify the mainboard (which is the exception), it might be necessary to -specify the mainboard using the -.B -m -switch (see above). +identify the mainboard (which is the exception), or if you want to override +the detected mainboard model, you can specify the mainboard using the .sp +.B " flashrom -p internal:mainboard=[<vendor>:]<board>" +syntax. +.sp +See the 'Known boards' or 'Known laptops' section in the output +of 'flashrom -L' for a list of boards which require the specification of +the board name, if no coreboot table is found. +.sp Some of these board-specific flash enabling functions (called .BR "board enables" ) in flashrom have not yet been tested. If your mainboard is detected needing Index: flashrom-kill_cli_mainboard_parameter/programmer.h =================================================================== --- flashrom-kill_cli_mainboard_parameter/programmer.h (Revision 1482) +++ flashrom-kill_cli_mainboard_parameter/programmer.h (Arbeitskopie) @@ -264,7 +264,7 @@ void cleanup_cpu_msr(void);
/* cbtable.c */ -void lb_vendor_dev_from_string(char *boardstring); +void lb_vendor_dev_from_string(const char *boardstring); int coreboot_init(void); extern char *lb_part, *lb_vendor; extern int partvendor_from_cbtable; Index: flashrom-kill_cli_mainboard_parameter/print.c =================================================================== --- flashrom-kill_cli_mainboard_parameter/print.c (Revision 1482) +++ flashrom-kill_cli_mainboard_parameter/print.c (Arbeitskopie) @@ -389,7 +389,10 @@ for (i = strlen("Board"); i < maxboardlen; i++) msg_ginfo(" ");
- msg_ginfo("Status Required option\n\n"); + msg_ginfo("Status Required option for\n"); + for (i = 0; i < maxvendorlen + maxboardlen + strlen("Status "); i++) + msg_ginfo(" "); + msg_ginfo("-p internal:mainboard=\n");
for (b = boards; b->vendor != NULL; b++) { msg_ginfo("%s", b->vendor); @@ -407,7 +410,7 @@ if (e->lb_vendor == NULL) msg_ginfo("(autodetected)"); else - msg_ginfo("-m %s:%s", e->lb_vendor, + msg_ginfo("%s:%s", e->lb_vendor, e->lb_part); } msg_ginfo("\n"); Index: flashrom-kill_cli_mainboard_parameter/board_enable.c =================================================================== --- flashrom-kill_cli_mainboard_parameter/board_enable.c (Revision 1482) +++ flashrom-kill_cli_mainboard_parameter/board_enable.c (Arbeitskopie) @@ -2070,7 +2070,8 @@ * The coreboot ids are used two fold. When running with a coreboot firmware, * the ids uniquely matches the coreboot board identification string. When a * legacy bios is installed and when autodetection is not possible, these ids - * can be used to identify the board through the -m command line argument. + * can be used to identify the board through the -p internal:mainboard= + * programmer parameter. * * When a board is identified through its coreboot ids (in both cases), the * main pci ids are still required to match, as a safeguard. @@ -2245,7 +2246,8 @@ msg_pinfo("AMBIGUOUS BOARD NAME: %s\n", part); msg_pinfo("At least vendors '%s' and '%s' match.\n", partmatch->lb_vendor, board->lb_vendor); - msg_perr("Please use the full -m vendor:part syntax.\n"); + msg_perr("Please use the full -p internal:mainboard=" + "vendor:part syntax.\n"); return NULL; } partmatch = board; @@ -2259,7 +2261,8 @@ * coreboot table. If it was, the coreboot implementor is * expected to fix flashrom, too. */ - msg_perr("\nUnknown vendor:board from -m option: %s:%s\n\n", + msg_perr("\nUnknown vendor:board from -p internal:mainboard=" + " programmer parameter:\n%s:%s\n\n", vendor, part); } return NULL;