Re: [flashrom] [PATCH] Use extract_param everywhere

[Carl-Daniel Hailfinger]

On 05.07.2010 17:01, Michael Karcher wrote:
> 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.

Absolutely agreed. Especially for libflashrom, that is essential.
> > ===================================================================
> > --- 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?

Good point. Should I do that in a followup patch or in an update of this
patch?
> > 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".

True. Well, if my docs are correct, strtol("123foo456") will return 123.
If we really want error checking for the parameter, we probably need to
check endptr in strtol, and should check if the value is nonzero and
less than 65536 as well. Maybe even a check for alignment...
The IT8705F support patch touches that area, and I'll extend it to take
care of this issue as well.
> > 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?

Yes. This allows us to abort init in case of broken parameters even
before we start to touch the hardware. The biggest benefit is not having
to shut down the serial port due to errors in parameters.
> > ===================================================================
> > --- 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()

Mh. You have a point. Given that the whole parameter handling stuff
strips out the extracted parameters from programmer_param, the string
should be empty in the end. Not sure if a single line checking for a
non-empty programmer_param should be in its own function, though.
> The rest seems fine, but I didn't test it.

Thanks for the review. How should I proceed?

Regards,
Carl-Daniel