From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:58776) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TsbxD-0004Bc-Aw for qemu-devel@nongnu.org; Tue, 08 Jan 2013 11:23:08 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TsbxB-00022j-NP for qemu-devel@nongnu.org; Tue, 08 Jan 2013 11:23:07 -0500 Received: from e9.ny.us.ibm.com ([32.97.182.139]:53998) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TsbxB-00022X-JG for qemu-devel@nongnu.org; Tue, 08 Jan 2013 11:23:05 -0500 Received: from /spool/local by e9.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 8 Jan 2013 11:23:03 -0500 Received: from d01relay03.pok.ibm.com (d01relay03.pok.ibm.com [9.56.227.235]) by d01dlp01.pok.ibm.com (Postfix) with ESMTP id 39E5B38C804D for ; Tue, 8 Jan 2013 11:23:00 -0500 (EST) Received: from d01av04.pok.ibm.com (d01av04.pok.ibm.com [9.56.224.64]) by d01relay03.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r08GMxQG308954 for ; Tue, 8 Jan 2013 11:22:59 -0500 Received: from d01av04.pok.ibm.com (loopback [127.0.0.1]) by d01av04.pok.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r08GMx7d031631 for ; Tue, 8 Jan 2013 11:22:59 -0500 From: Anthony Liguori In-Reply-To: References: <1357584074-10852-1-git-send-email-fred.konrad@greensocs.com> <874nisu7tp.fsf@codemonkey.ws> Date: Tue, 08 Jan 2013 10:22:50 -0600 Message-ID: <87lic3zlt1.fsf@codemonkey.ws> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Subject: Re: [Qemu-devel] [PATCH 00/61] Virtio refactoring. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: kwolf@redhat.com, e.voevodin@samsung.com, mst@redhat.com, mark.burton@greensocs.com, agraf@suse.de, qemu-devel@nongnu.org, amit.shah@redhat.com, aneesh.kumar@linux.vnet.ibm.com, stefanha@redhat.com, cornelia.huck@de.ibm.com, pbonzini@redhat.com, afaerber@suse.de, fred.konrad@greensocs.com Peter Maydell writes: > On 7 January 2013 19:11, Anthony Liguori wrote: >> I appreciate the thoroughness here but 66 patches is too much all at >> once. Can this be done in chunks or more reasonable patch sizes? It >> would make review and committing a lot easier. > > So do you have any suggestions? The patchset is long but it > is actually in a fairly easily separable set of sections: > Sounds like you have already identified how to break things up... > I: Initial class definitions and transport level refactoring: > >>> qdev : add a maximum device allowed field for the bus. >>> virtio-bus : introduce virtio-bus >>> virtio-pci-bus : introduce virtio-pci-bus. >>> virtio-pci : refactor virtio-pci device. >>> virtio-device : refactor virtio-device. >>> virtio-s390-bus : add virtio-s390-bus. >>> virtio-s390-device : create a virtio-s390-bus during init. I would start with the above. > > II: Update virtio-blk: > >>> virtio-blk : show VirtIOBlock structure. >>> virtio-blk : don't use pointer for configuration. >>> virtio-blk : add the virtio-blk device. >>> virtio-blk-pci : switch to new API. >>> virtio-blk-s390 : switch to the new API. >>> virtio-blk : cleanup : use QOM cast. >>> virtio-blk : cleanup : remove qdev field. > > III: Update virtio-net: > >>> virtio-net : show the VirtIONet structure. >>> virtio-net : add the virtio-net device. >>> virtio-net-pci : switch to the new API. >>> virtio-net-s390 : switch to the new API. >>> virtio-net : cleanup : use QOM cast. >>> virtio-net : cleanup : init and exit function. >>> virtio-net : cleanup : remove qdev field. > > [etc; all the backends are handled in basically the same way] I would do all of the devices at once. > > N: Final cleanup now all backends are converted: > >>> virtio : remove the function pointer. >>> virtio-pci : cleanup : init, exit and reset functions. >>> s390-virtio-bus : cleanup >>> virtio : remove virtiobindings. >>> virtio : cleanup : init and exit function. I would do this independently. Regards, Anthony Liguori > > where I guess the interesting bits to review in particular > would be phases I and N and a randomly selected backend. > > Perhaps you could squash some of the patches together, > for instance the "virtio-foo: show the VirtIOFoo structure" > (which is just moving a struct from a private .c file to the .h) > could go in with the following "virtio-net : add the virtio-foo > device." patch, which would reduce the patch count by > seven or so -- is that worth doing? (obviously it's the same > amount of actual code just in fewer patches...) > > thanks > -- PMM