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