Re: [Qemu-devel] [PATCH 00/61] Virtio refactoring.

[Anthony Liguori]

Peter Maydell <address@hidden> writes:

> On 7 January 2013 19:11, Anthony Liguori <address@hidden> 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