From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1NfAAP-0008NI-HN for qemu-devel@nongnu.org; Wed, 10 Feb 2010 05:51:33 -0500 Received: from [199.232.76.173] (port=45268 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1NfAAO-0008NA-QG for qemu-devel@nongnu.org; Wed, 10 Feb 2010 05:51:32 -0500 Received: from Debian-exim by monty-python.gnu.org with spam-scanned (Exim 4.60) (envelope-from ) id 1NfAAN-0005JW-Cr for qemu-devel@nongnu.org; Wed, 10 Feb 2010 05:51:32 -0500 Received: from mx1.redhat.com ([209.132.183.28]:1025) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1NfAAN-0005JS-1X for qemu-devel@nongnu.org; Wed, 10 Feb 2010 05:51:31 -0500 Date: Wed, 10 Feb 2010 12:48:13 +0200 From: "Michael S. Tsirkin" Subject: Re: [Qemu-devel] Re: [PATCH 09/15] eepro100: convert to new pci interface Message-ID: <20100210104813.GA16789@redhat.com> References: <1265752899-26980-1-git-send-email-aliguori@us.ibm.com> <1265752899-26980-10-git-send-email-aliguori@us.ibm.com> <4B7252EA.7000300@mail.berlios.de> <20100210094929.GF14063@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: Anthony Liguori , qemu-devel@nongnu.org On Wed, Feb 10, 2010 at 11:43:29AM +0100, Markus Armbruster wrote: > "Michael S. Tsirkin" writes: > > > On Wed, Feb 10, 2010 at 07:32:10AM +0100, Stefan Weil wrote: > >> See my inline comments. > >> > >> > >> Anthony Liguori schrieb: > [...] > >> > -static void pci_map(PCIDevice * pci_dev, int region_num, > >> > - pcibus_t addr, pcibus_t size, int type) > >> > +static void pci_io_write(PCIDevice *dev, pcibus_t addr, int size, > >> > + uint32_t value) > >> > { > >> > - EEPRO100State *s = DO_UPCAST(EEPRO100State, dev, pci_dev); > >> > - > >> > - TRACE(OTHER, logout("region %d, addr=0x%08"FMT_PCIBUS", " > >> > - "size=0x%08"FMT_PCIBUS", type=%d\n", > >> > - region_num, addr, size, type)); > >> > + EEPRO100State *s = DO_UPCAST(EEPRO100State, dev, dev); > >> > > >> > >> Please don't change the name of the PCIDevice pointer argument > >> from pci_dev to dev. > >> > >> This dev, dev in DO_UPCAST is ugly and misleading. > > > > It's a matter of taste: I actually think it's pretty: I never remember > > which argument of DO_UPCAST is which, and if both are dev it doesn't > > matter. > > I also have trouble remembering how to use DO_UPCAST(), but playing > games with identifiers is a poor fix for that. I find the use of the > same name "dev" for two different things in the same expression rather > confusing. Could we improve DO_UPCAST() instead? > > For what it's worth, I have no trouble with container_of(). DO_UPCAST() > takes the same arguments in a different order, which I find irritating. > > [...] IMO it would be ideal to remove offset assumptions where possible and then we can use container_of. -- MST