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

[Stefan Tauner]

On Tue, 03 Jan 2012 01:04:37 +0100
Carl-Daniel Hailfinger  wrote:
> Am 02.01.2012 11:29 schrieb 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
> >
> > 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.
>
> 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.

well that's not the only problem imo. you can't free tempstr like that,
because it gets modified by strtok. i am not 100% sure what your
version would do actually. the address tempstr points to, may be
one that was not (the start of) an allocated area. what does
calling free on that do according to the standard?

please look further below for my version which saves the original
address of tempstr in a new variable and frees that one at the end.
> 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.

missing in this iteration
> > the whole cb string handling is a bit messy.
>
> Understatement of the week.

oh well, i rant so much so i have to be comforting and polite at least
SOMETIMES :)
> > 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
>
> […]
> 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)
> […]
> @@ -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);

imho this dual use is wrong. if we would not have the override switch,
it would be somewhat ok, but since we do, the mainboard= values should
be only hints. what does the code do if boardmismatch=force AND
mainboard=... is given? just from the message above, i can't tell (not
that i should be able to, but it suggests an inconsistency.
> 	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 " : "&mdash;",
>        b[k].lb_vendor ? "-p internal:mainboard= " : "&mdash;",

the space at the end is wrong for sure.
i have looked at the output and it of course makes the column larger, a
bit larger than the model name column (which is quite huge :)
i think it is reasonable though. the whole table is split into two
main columns so there is plenty of screen space before this patch, and
still some after (i.e. there are wider tables in the output).
>        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 *freestr = tempstr;
> 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);

instead:
+	free(freestr);
> }
>
> static unsigned long compute_checksum(void *addr, unsigned long length)
> […]
> 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 me the option is "mainboard" and the specific mainboard strings are
the parameter for this option. hence i would write "Required parameter
for" here. The format looks ok on my machine.
> for (i = 0; i < maxvendorlen + maxboardlen + strlen("Status  "); i++)
> msg_ginfo(" ");
> msg_ginfo("-p internal:mainboard=\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.

here it is the other way round from above... imo: "option" instead of
"parameter" because the sentence references the "mainboard=" part,
which to me is the option.
no idea if there is a textbook definition for this stuff... or if
anyone besides me cares :)
> 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",

ditto

apart from the free call in cbtable.c (+ the missing comment) and the
additional space in the wiki output this is
Acked-by: Stefan Tauner 
NB: i have not verified row length limitations in code or output, or
the resulting manpage layout. also, i have not rechecked for forgotten
-m or --mainboard references this time. :)