Re: [flashrom] [PATCH] Use extract_param everywhere

[Michael Karcher]

Am Samstag, den 03.07.2010, 03:25 +0200 schrieb Carl-Daniel Hailfinger:
> Add two instances of exit(1) where we have no option to return an error.
> Remove six instances of exit(1) where returning an error was possible.

Long term goal would be to add the option to return an error to the
places you add the exit(1), of course.
> ===================================================================
> --- flashrom-extract_param_everywhere/drkaiser.c	(Revision 1067)
> +++ flashrom-extract_param_everywhere/drkaiser.c	(Arbeitskopie)
> @@ -36,10 +36,14 @@
>  int drkaiser_init(void)
>  {
>    uint32_t addr;
>
> char *devstring;
>
> get_io_perms();
>
> devstring = extract_param(&programmer_param, "pci", ",");
> pcidev_init(PCI_VENDOR_ID_DRKAISER, PCI_BASE_ADDRESS_2,
>     drkaiser_pcidev, programmer_param);
>     drkaiser_pcidev, devstring);
> free(devstring);

can't we do the devsting stuff inside the pcidev_init function, if it is
the same for all programmers?
> if (programmer_param && !strlen(programmer_param)) {
> 	free(programmer_param);
> 	programmer_param = NULL;
> /* Non-default port requested? */
> portpos = extract_param(&programmer_param, "it87spiport", ",:");
> if (portpos && strlen(portpos)) {
> 	flashport = strtol(portpos, (char **)NULL, 0);

I know it wasn't in before, but some minimal error checking would be
really nice here. strtol returns 0 on failure, and we don't want to set
port 0. Think of "it87spiport=0y4800".
> ret = buspirate_serialport_setup(dev);
> if (ret)
> return ret;
> free(dev);
> speed = extract_param(&programmer_param, "spispeed", ",:");
> if (speed) {
> for (i = 0; spispeeds[i].name; i++)
> 	if (!strncasecmp(spispeeds[i].name, speed,
>
> @@ -129,13 +123,11 @@
>    	if (!spispeeds[i].name)
>    		msg_perr("Invalid SPI speed, using default.\n");
>    }
> free(speed);
> /* This works because speeds numbering starts at 0 and is contiguous. */
>  msg_pdbg("SPI speed is %sHz\n", spispeeds[spispeed].name);
> ret = buspirate_serialport_setup(dev);
> if (ret)
> return ret;

This hunk moves the initialization of the device. Is that intended?
> ===================================================================
> --- flashrom-extract_param_everywhere/flashrom.c	(Revision 1067)
> +++ flashrom-extract_param_everywhere/flashrom.c	(Arbeitskopie)
> @@ -428,7 +428,15 @@
>
> int programmer_init(void)
>  {
> return programmer_table[programmer].init();
> int ret;
> ret = programmer_table[programmer].init();
> if (programmer_param && strlen(programmer_param)) {

In the end, this needs to be in a subfunction like
get_unhandled_parameters()

The rest seems fine, but I didn't test it.

Regards,
  Michael Karcher