Re: [flashrom] [PATCH] Replace --mainboard with -p internal:mainboard

[Stefan Tauner]

On Mon, 02 Jan 2012 03:48:40 +0100
Carl-Daniel Hailfinger  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
> 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)
> @@ -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;
> 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;

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 = tempstr2;
> } else {
> lb_vendor = NULL;
> 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.

please add a function comment explaining that stuff.
> }
> 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)
> @@ -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;

what about this in cli_classic.c (seems to be completely useless now):
#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

and this in board_enable.c:
    		msg_perr("Please use the full -m vendor:part syntax.\n");
and
    	msg_perr("\nUnknown vendor:board from -m option: %s:%s\n\n",
    		 vendor, part);

and this in print.c:
    			msg_ginfo("-m %s:%s", e->lb_vendor,
    					   e->lb_part);
and this in flashrom.8:
         [\fB-c\fR <chipname>] [\fB-m\fR [<vendor>:]<board>] \

aaaand... please use grep -- -m yourself, there are other bits
referring to the old syntax.

the whole cb string handling is a bit messy.
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
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 ;)