From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43)
	id 1LXeGc-00018n-Hl
	for qemu-devel@nongnu.org; Thu, 12 Feb 2009 11:18:22 -0500
Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43)
	id 1LXeGa-000177-Ug
	for qemu-devel@nongnu.org; Thu, 12 Feb 2009 11:18:22 -0500
Received: from [199.232.76.173] (port=37274 helo=monty-python.gnu.org)
	by lists.gnu.org with esmtp (Exim 4.43) id 1LXeGa-00016y-Qm
	for qemu-devel@nongnu.org; Thu, 12 Feb 2009 11:18:20 -0500
Received: from mx20.gnu.org ([199.232.41.8]:12945)
	by monty-python.gnu.org with esmtps (TLS-1.0:RSA_AES_256_CBC_SHA1:32)
	(Exim 4.60) (envelope-from <paul@codesourcery.com>)
	id 1LXeGa-0008G8-Ky
	for qemu-devel@nongnu.org; Thu, 12 Feb 2009 11:18:20 -0500
Received: from mail.codesourcery.com ([65.74.133.4])
	by mx20.gnu.org with esmtp (Exim 4.60)
	(envelope-from <paul@codesourcery.com>) id 1LXeGZ-0004VL-Nj
	for qemu-devel@nongnu.org; Thu, 12 Feb 2009 11:18:20 -0500
From: Paul Brook <paul@codesourcery.com>
Subject: Re: [Qemu-devel] [6609] Parse full PCI device addresses (Markus
	Armbruster)
Date: Thu, 12 Feb 2009 16:18:16 +0000
References: <E1LXGuL-0004v2-7h@cvs.savannah.gnu.org>
	<200902121539.26892.paul@codesourcery.com>
	<20090212155605.GA18654@amt.cnet>
In-Reply-To: <20090212155605.GA18654@amt.cnet>
MIME-Version: 1.0
Content-Type: text/plain;
  charset="iso-8859-1"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline
Message-Id: <200902121618.17411.paul@codesourcery.com>
Reply-To: qemu-devel@nongnu.org
List-Id: qemu-devel.nongnu.org
List-Unsubscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.gnu.org/pipermail/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: qemu-devel@nongnu.org
Cc: Marcelo Tosatti <mtosatti@redhat.com>, Markus Armbruster <armbru@redhat.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