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

[Marcelo Tosatti]

On Thu, Feb 12, 2009 at 04:18:16PM +0000, Paul Brook wrote:
> > > 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.

That makes sense.

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

OK, i'll submit something later in the week so you can ACK/NACK. We want
to pass <dom,bus,slot> all the way down to pci_register_device, and kill
the "-1" parameter that drivers use today (so one can statically assign
bus/slot and eventually domain).

But OK, will drop domain from the API for now.