From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1NsBkc-0005aS-PF for qemu-devel@nongnu.org; Thu, 18 Mar 2010 05:10:46 -0400 Received: from [199.232.76.173] (port=48557 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1NsBkb-0005ZN-Dc for qemu-devel@nongnu.org; Thu, 18 Mar 2010 05:10:45 -0400 Received: from Debian-exim by monty-python.gnu.org with spam-scanned (Exim 4.60) (envelope-from ) id 1NsBkZ-0001LG-N6 for qemu-devel@nongnu.org; Thu, 18 Mar 2010 05:10:45 -0400 Received: from mx1.redhat.com ([209.132.183.28]:10183) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1NsBkZ-0001LC-Ay for qemu-devel@nongnu.org; Thu, 18 Mar 2010 05:10:43 -0400 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id o2I9AgQv008394 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 18 Mar 2010 05:10:42 -0400 Date: Thu, 18 Mar 2010 11:07:11 +0200 From: "Michael S. Tsirkin" Message-ID: <20100318090711.GB23649@redhat.com> References: <20100318064015.GA16973@redhat.com> <20100318074239.GA23474@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: [Qemu-devel] Re: [PATCH 0/9] Virtio cleanups List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Juan Quintela Cc: qemu-devel@nongnu.org On Thu, Mar 18, 2010 at 09:36:15AM +0100, Juan Quintela wrote: > "Michael S. Tsirkin" wrote: > > >> Look at the rest of hw/*. > > > > I think you mean that a similar assumption is made by lots of > > other code. Does not mean it's always a good idea and that > > we need to force this assumption everywhere. > > Principle of Least Surprise. as posted in the other email, removing > that assumption don't bring us anything and just make code more complex > (not a huge ammount, but more complex). > Well, you can not put both vdev and qdev in the same device as both want to be the first field. If you try, you get a big surprise :) > >> The only "sane" way of doing OOP on C is to use the super-class memmbers as the > >> 1st member of the sub-classes. That will not change anytime soon. And > >> trying to "emulate" multiple inheritance in C is completely "insane". > > > > container_of does it and I think it's sane. > > People like comparing it to inheritance but for me it's just > > using a field in a structure. multiple fields in a structure are insane? > > It is inheritance. > > we assume in virtio_common_init() can be called for all virtio devices. > That means that all virtio devices have "something" in common. > That is the basic concept of super-class and sub-class. Yes, C sucks > for describing that kind of relationships, but that is a different matter. > > >> This "assumption" is used for all the > >> code left and right. It is an essentioal part of the qdev design, not > >> something that can be changed easily. > > > > I do not think it is essential at all. We just add an offset. > > Yes we make such assumptions a lot but it's a bug, not a feature. > > Clearly, we don't agree here. For me it is a "feature" that all virtio > devices are in the way: > > struct virtio_common { > ... > } > > struct virtio_specific { > struct virtio_common common; > > } > > And then I can pass virtio_specific pointers to virtio_common routines > and things just work. With casts? Why not pass in &specific->common? > Special importance when we have callbacks, and > virtio_specific parts are only used in virtio_specific.c file. You need a cast anyway: container_of or UP_CAST. With layout assumptions people tend to be lazy and use C casts, and it kind of works, until it breaks in surprising ways. > Why do you want to break that assumption that is used (in a good place) > in a lot of qemu code (qdev "requires" it)? Not break, lift the assumption. > For me is a clear case of > coherence. Virtio* can live with container_of() instead of DO_UPCAST() > because they are not exposed (directly) through qdev. Then mark it as > different that everything else, indeed if it don't bring us anything, > just to confuse new users. This is one of the features that I hate > more, searching for how to use a qemu api because from only the > prototypes it is not clear, and just pick an example, and that one uses > it in a non-conventional way. > > Later, Juan. I think we should just fix qdev. All we need to do is replace .qdev.size = sizeof(VirtIOPCIProxy), with DEFINE_QDEV(VirtIOPCIProxy, qdev), -- MST