Re: [flashrom] [PATCH] dummy programmer: blacklist SPI commands

[Maciej Pijanka]

On Wed, 19 Jan 2011, Carl-Daniel Hailfinger wrote:
> New version with additional bugfixes and man page.
> The man page formatting is broken, I'd appreciate help from someone who
> knows how to fix the formatting.

What is wrong with manpage formatting, of course besides content, few parts could
use some work.
> Add optional SPI command blacklisting to the flash chip emulator in the
> dummy programmer.
>
> Usage:
> flashrom -p dummy:spi_blacklist=hexstring
>
> If hexstring is 0302, flashrom will blacklist command 0x03 (READ) and
> command 0x02 (WRITE). The hexstring can have up to 521 bytes (256
> commands) length, and must not start with 0x.
>
> TODO: Should 0x at the beginning of hexstring be tolerated?

Above remark seems to be inaccurate or outdated, as code skips 0x if its on
start of string.
> Very useful for testing corner cases if you don't own a locked down
> Intel chipset and want to simulate such a thing.
>
> Fix out-of-bounds access in the simulator command checking code.
>
> Signed-off-by: Carl-Daniel Hailfinger
>
> Index: flashrom-emulate_spi_flashchip_command_blacklist/dummyflasher.c
> tmp = extract_programmer_param("spi_blacklist");
> if (tmp) {

Is there any reason why tmp is declared for whole function instead as variable used only in this block?
Also i would rather name 'tmp' variable 'blacklist_param' or something similar, it isnt reused after as
anything else and would be easier to read code knowing for what it is used for. Also other parameters
have more meaningfull variable names.
> tolower_string(tmp);
> tmp2 = tmp;
> if (!strncmp(tmp2, "0x", 2)) {
> 	tmp2 += 2;
> }
> i = strlen(tmp2);
> if ((i > 512) || (i % 2)) {
> 	msg_perr("Invalid SPI command blacklist length\n");
> 	free(tmp);
> 	return 1;
> }
> if (strspn(tmp2, "0123456789abcdef") != i) {
> 	msg_perr("Invalid chars in SPI command blacklist\n");
> 	free(tmp);
> 	return 1;
> }
> spi_blacklist_size = i / 2;
> for (i = 0; i < spi_blacklist_size; i++) {
> 	sscanf(tmp2 + i * 2, "%2hhx", &spi_blacklist[i]);
> }
> msg_pdbg("SPI blacklist is ");
> for (i = 0; i < spi_blacklist_size; i++)
> 	msg_pdbg("%02x ", spi_blacklist[i]);
> msg_pdbg(", size %i\n", spi_blacklist_size);
> }
> free(tmp);
> /* TODO: Implement command blacklists here. */
> for (i = 0; i < spi_blacklist_size; i++) {
> if (writearr[0] == spi_blacklist[i]) {
> 	msg_perr("Refusing blacklisted SPI command 0x%02x\n",
> 		 spi_blacklist[i]);
> 	/* FIXME: Do we want a separate return code for
> 	 * command unavailable?
> 	 */

I think, only if non-emulated write may return command unavailable response.
> @@ -444,12 +487,9 @@
>    		msg_perr("CHIP ERASE 0x60 insize invalid!\n");
>    		return 1;
>    	}
> offs = writearr[1] << 16 | writearr[2] << 8 | writearr[3];
> if (offs & (emu_jedec_ce_60_size - 1))
> 	msg_pdbg("Unaligned CHIP ERASE 0x60\n");
> @@ -462,10 +502,7 @@
>    		msg_perr("CHIP ERASE 0xc7 insize invalid!\n");
>    		return 1;
>    	}
> offs = writearr[1] << 16 | writearr[2] << 8 | writearr[3];
> if (offs & (emu_jedec_ce_c7_size - 1))
> 	msg_pdbg("Unaligned CHIP ERASE 0xc7\n");

Any word of explain why those two debug messages above are removed?
> Index: flashrom-emulate_spi_flashchip_command_blacklist/flashrom.8
> +If hexstring is 0302, flashrom will blacklist command 0x03 (READ) and
> +command 0x02 (WRITE). The hexstring can have up to 512 characters (256
> +commands) length, and must not start with 0x.

and should contain 0-9a-fA-F characters only? (0x on begin of whole string
is ignored anyway).

I think, along with more options for dummy, manpage could get example how
to pass both, blacklist and something else as parameters for dummy programmer.

sth like -pdummy=SST25VF032B,spi_blacklist=ffeeaa

A bit changed manpage part of patch is in attachment.