Re: [flashrom] [PATCH 2/4] Refactor PCI and USB device status printing.

[Carl-Daniel Hailfinger]

Am 25.12.2012 12:31 schrieb Stefan Tauner:
> To be able to get rid of lots of #ifdefs and centralize programmer-specific
> data more...
> introduce two new fields to struct programmer_entry, namely
> enum type (OTHER, USB, PCI) and union devs (pcidev_status, usbdev_status
> or char *note).
> use those fields to generate device listings in print.c and print_wiki.c.
>
> Bonus: add printing of USB devices to print_wiki.c and count supported PCI
> and USB devices.
>
> Signed-off-by: Stefan Tauner

Thanks for your patch!
> diff --git a/flashrom.c b/flashrom.c
> index 77cae7c..b585dc4 100644
> --- a/flashrom.c
> +++ b/flashrom.c
> @@ -1576,6 +1622,35 @@ int selfcheck(void)
>    		 * messages below without jumping through hoops. */
>    		continue;
>    	}
> switch (p.type) {
> case USB:
> 	if (p.devs.usb == NULL) {
> 		msg_gerr("Programmer %s is of type USB, but its device list is NULL!\n",
> 			 p.name);
> 		ret = 1;
> 	}
> 	break;
>
> +#if NEED_PCI == 1
> case PCI:
> 	if (p.devs.pci == NULL) {
> 		msg_gerr("Programmer %s is of type PCI, but its device list is NULL!\n",
> 			 p.name);
> 		ret = 1;
> 	}
> 	break;
>
> +#endif
> case OTHER:
> 	if (p.devs.note == NULL) {
> 		if (strcmp("internal", p.name) == 0)
> 			break; // this is expected, no worry
> 		msg_gerr("Programmer %s is of type OTHER, but its note is NULL!\n", p.name);
> 		ret = 1;
> 	}
> 	break;

Can't you simply handle USB/PCI/OTHER without switch()? After all,
devs.pci, devs.usb and devs.note are in a union and you can't detect
bugs like .type=USB combined with .devs.note="foobar" anyway. The only
special case is the internal programmer. Suggestion:

switch(p.type) {
case USB:
case PCI:
case OTHER:
    /* This is a union, check only one member. */
    if (p.devs.note == NULL) {
        if (strcmp("internal", p.name) == 0)
            break; /* This one has its device list stored separately. */
        msg_gerr("Programmer %s has neither a device list nor a textual
description!\n", p.name);
        ret = 1;
    break;
default:
...
> default:
> 	msg_gerr("Programmer %s does not have a valid type set!\n", p.name);
> 	break;
> }
> if (p.init == NULL) {
> 	msg_gerr("Programmer %s does not have a valid init function!\n", p.name);
> 	ret = 1;

Rest looks ok.

Acked-by: Carl-Daniel Hailfinger

Regards,
Carl-Daniel

[PATCH 2/4] Refactor PCI and USB device status printing.

[Stefan Tauner]

On Wed, 26 Dec 2012 20:32:35 +0100
Carl-Daniel Hailfinger  wrote:
> Can't you simply handle USB/PCI/OTHER without switch()? After all,
> devs.pci, devs.usb and devs.note are in a union and you can't detect
> bugs like .type=USB combined with .devs.note="foobar" anyway. The only
> special case is the internal programmer. Suggestion:
>
> switch(p.type) {
> case USB:
> case PCI:
> case OTHER:
>     /* This is a union, check only one member. */
>     if (p.devs.note == NULL) {
>         if (strcmp("internal", p.name) == 0)
>             break; /* This one has its device list stored separately. */
>         msg_gerr("Programmer %s has neither a device list nor a textual
> description!\n", p.name);
>         ret = 1;
>     break;
> default:
> ...

that's certainly possible. the rationale for my code (besides its
development while hacking around and refining) is:
the detailed messages could help the programmer writer better than a
generic message that catches all.
the question is if that is worth it...

[PATCH 2/4] Refactor PCI and USB device status printing.

[Carl-Daniel Hailfinger]

Am 26.12.2012 20:44 schrieb Stefan Tauner:
> On Wed, 26 Dec 2012 20:32:35 +0100
> Carl-Daniel Hailfinger  wrote:
> > Can't you simply handle USB/PCI/OTHER without switch()? After all,
> > devs.pci, devs.usb and devs.note are in a union and you can't detect
> > bugs like .type=USB combined with .devs.note="foobar" anyway. The only
> > special case is the internal programmer. Suggestion:
> >
> > switch(p.type) {
> > case USB:
> > case PCI:
> > case OTHER:
> >     /* This is a union, check only one member. */
> >     if (p.devs.note == NULL) {
> >         if (strcmp("internal", p.name) == 0)
> >             break; /* This one has its device list stored separately. */
> >         msg_gerr("Programmer %s has neither a device list nor a textual
> > description!\n", p.name);
> >         ret = 1;
> >     break;
> > default:
> > ...
>
> that's certainly possible. the rationale for my code (besides its
> development while hacking around and refining) is:
> the detailed messages could help the programmer writer better than a
> generic message that catches all.
> the question is if that is worth it...

I don't think it's worth it. My suggested error message is not optimal,
though. It may have been misunderstood as "please add a device list or
textual description, your choice" which would be clearly wrong. What about

"Programmer %s has no device list/textual description of the right type!\n"

Regards,
Carl-Daniel

[PATCH 2/4] Refactor PCI and USB device status printing.

[Stefan Tauner]

On Wed, 26 Dec 2012 21:04:41 +0100
Carl-Daniel Hailfinger  wrote:
> I don't think it's worth it. My suggested error message is not optimal,
> though. It may have been misunderstood as "please add a device list or
> textual description, your choice" which would be clearly wrong. What about
>
> "Programmer %s has no device list/textual description of the right type!\n"

that sounds a bit as if it is set but only the type is wrong IMHO.
yay, overengineering a message no one should ever see. i missed you ;)

[PATCH 2/4] Refactor PCI and USB device status printing.

[Stefan Tauner]

On Wed, 26 Dec 2012 21:30:11 +0100
Stefan Tauner  wrote:
> On Wed, 26 Dec 2012 21:04:41 +0100
> Carl-Daniel Hailfinger  wrote:
> > I don't think it's worth it. My suggested error message is not optimal,
> > though. It may have been misunderstood as "please add a device list or
> > textual description, your choice" which would be clearly wrong. What about
> >
> > "Programmer %s has no device list/textual description of the right type!\n"
>
> that sounds a bit as if it is set but only the type is wrong IMHO.

therefore I'll use your first suggestion:
"Programmer %s has neither a device list nor a textual description!\n"
> yay, overengineering a message no one should ever see. i missed you ;)

and that was only slightly sarcastic, i really appreciate such
feedback. :)

[PATCH 2/4] Refactor PCI and USB device status printing.

[Stefan Tauner]

On Wed, 26 Dec 2012 20:32:35 +0100
Carl-Daniel Hailfinger  wrote:
> Acked-by: Carl-Daniel Hailfinger

thx, r1631