Re: [Qemu-devel] [PATCH] virtio-net: require that config size is initialized

[Michael S. Tsirkin]

On Tue, May 07, 2013 at 05:29:34PM +0200, KONRAD Frédéric wrote:
> On 07/05/2013 16:00, Michael S. Tsirkin wrote:
> >On Tue, May 07, 2013 at 02:54:38PM +0200, KONRAD Frédéric wrote:
> >>On 07/05/2013 14:33, Michael S. Tsirkin wrote:
> >>>On Tue, May 07, 2013 at 02:29:38PM +0200, KONRAD Frédéric wrote:
> >>>>Hi,
> >>>>
> >>>>I think it is not a good idea, as we wanted to make VirtIODevice
> >>>>hot-pluggable for virtio-mmio.
> >>>And then this hack will break cross version migration.
> >>Why?
> >>
> >>virtio_net_set_config_size is called by each "proxy" devices no?
> >If it's called then there's no need for a default,
> >so this patch should be fine.
> 
> True but as I told you, we made this refactoring to be able to hot-plug
> VirtIODevice on a virtio-bus, so calling this function won't be possible.

Going in circles aren't we? If it's not called
it's a bug. If it's called default is not needed.

> But you are right this must be fixed.
> 
> >
> >>>>Maybe we can use a callback which is called by virtio-bus before
> >>>>plugging, to ensure that this
> >>>>config size is computed?
> >>>OK, will you post such a patch?
> >>>
> >>The issue is as we said in the last thread, host_feature is part of
> >>the proxy.
> >Maybe you could add documentation on how initialization works?
> >If I could fiure it out I would maybe suggest some solutions.
> 
> Yes, where do you think I can add this?

Everywhere. Seriously.
Start with files. File headers should give some hint
as to what's in here
 * VirtioBus
in virtio-bus.h and virtio-bus.c is not helpful.
How about

        VirtioBus - a bus that drives from Newcastle to Edinburgh
        each Tuesday at noon. Drives back on Wednesdays mostly empty.
        It serves as a parent class for all the small taxi cars
        in both of these towns.

or some such.

Go over the interfaces and document what they do.
I mean more than just repeat the name:
        /* Destroy the VirtIODevice */
        void virtio_bus_destroy_device(VirtioBusState *bus)
is not really helpful.

        /* Destroy a device. Assumes you
           don't have valuables in there.  Calls 911 automatically. */

same for

/* Plug the VirtIODevice */
int virtio_bus_plug_device(VirtIODevice *vdev)

did you mean

        /* Plug the device so the content does not
           spill. This way you can take put it on the bus.
           You must use a corkscrew to unplug it later.
         */


> >>And if we want to move it to the devices, we must have a kind of
> >>property forwarding mechanism.
> >>
> >>Anthony said he had something about that.
> >>>>>All buses do this, and if they don't they get subtle
> >>>>>bugs related to cross version migration.
> >>>>>Let's add an assert to catch these bugs early.
> >>>>>
> >>>>>Signed-off-by: Michael S. Tsirkin <address@hidden>
> >>>>>---
> >>>>>
> >>>>>Just a cleanup so not 1.5 material.
> >>>>>Seems to work fine here - any opinions?
> >>>>>
> >>>>>  hw/net/virtio-net.c | 7 ++++---
> >>>>>  1 file changed, 4 insertions(+), 3 deletions(-)
> >>>>>
> >>>>>diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> >>>>>index 908e7b8..f0a9272 100644
> >>>>>--- a/hw/net/virtio-net.c
> >>>>>+++ b/hw/net/virtio-net.c
> >>>>>@@ -1282,6 +1282,8 @@ static int virtio_net_device_init(VirtIODevice 
> >>>>>*vdev)
> >>>>>      DeviceState *qdev = DEVICE(vdev);
> >>>>>      VirtIONet *n = VIRTIO_NET(vdev);
> >>>>>+    /* Verify that config size has been initialized */
> >>>>>+    assert(n->config_size != (size_t)-1);
> >>>>>      virtio_init(VIRTIO_DEVICE(n), "virtio-net", VIRTIO_ID_NET,
> >>>>>                                    n->config_size);
> >>>>>@@ -1386,10 +1388,9 @@ static void virtio_net_instance_init(Object *obj)
> >>>>>      VirtIONet *n = VIRTIO_NET(obj);
> >>>>>      /*
> >>>>>-     * The default config_size is sizeof(struct virtio_net_config).
> >>>>>-     * Can be overriden with virtio_net_set_config_size.
> >>>>>+     * The config_size must be set later with 
> >>>>>virtio_net_set_config_size.
> >>>>>       */
> >>>>>-    n->config_size = sizeof(struct virtio_net_config);
> >>>>>+    n->config_size = (size_t)-1;
> >>>>>  }
> >>>>>  static Property virtio_net_properties[] = {