Re: [Qemu-devel] [6609] Parse full PCI device addresses (Markus Armbruster)

[Paul Brook <paul@codesourcery.com>]

> > If we're rejecting nonzero domains then having the parse routine return a
> > domain value seems wrong. It's just going to make it harder to verify
> > correct operation when domains are implemented.
>
> +    /* Note: QEMU doesn't implement domains other than 0 */
> +    if (dom != 0 || pci_find_bus(bus) == NULL) {
> +       fprintf(stderr, "PCI device address %s not supported", addr);
> +       return -1;
> +    }
>
> +    if (!strcmp(devaddr, "auto")) {
> +        *domp = *busp = 0;
> +        *slotp = -1;
> +        /* want to support dom/bus auto-assign at some point */
> +        return 0;
> +    }
>
> We return domain 0. I considered domain 0 as implicit at the moment, is
> that wrong?
>
> I can't see where you're getting at.

You're returning a value that's always known to be zero. IMHO That's worse 
than not returning a value at all.

This implies users of this function will either ignore the value or have 
redundant, untested (i.e. probably bitrotten) code to handle nonzero domains. 
Either way, it's liable the break horribly at runtime as soon as we start 
trying to support multiple domains.

If pci_parse_devaddr is defined to only handle zero domains, then it should 
not be returning a domain value. If/when we implement multiple PCI domains we 
can change the interface, and get nice compiler errors in all the other code 
we forgot to update.

Alternatively, have the parse routine return the full tuple, and enforce 
domain == 0 elsewhere.

Paul