[Qemu-devel] Re: [PATCH 33/41] virtio-net: port to vmstate

[Michael S. Tsirkin]

On Wed, Dec 02, 2009 at 01:04:31PM +0100, Juan Quintela wrote:
> 
> Signed-off-by: Juan Quintela <address@hidden>
> ---
>  hw/virtio-net.c |  148 
> ++++++++++++++++++++++++-------------------------------
>  1 files changed, 64 insertions(+), 84 deletions(-)
> 
> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> index 4434827..3a59449 100644
> --- a/hw/virtio-net.c
> +++ b/hw/virtio-net.c
> @@ -703,6 +703,38 @@ static void virtio_net_tx_timer(void *opaque)
>      virtio_net_flush_tx(n, n->tx_vq);
>  }
> 
> +/* Restore an uint8_t from an uint32_t
> +   This is a Big hack, but it is how the old state did it.
> + */

I do not see why this is such a big hack.
u8 fits in u32. there might be many reasons
to use u32 for savevm (e.g. to be forward
compatible) and still use u8 at runtime
(e.g. to save memory).

IMO we should teach infrastructure to cope
wit this gracefully without littering
code with _hack suffixes.

> +
> +static int get_uint8_from_uint32(QEMUFile *f, void *pv, size_t size)
> +{
> +    uint8_t *v = pv;
> +    *v = qemu_get_be32(f);
> +    return 0;
> +}
> +
> +static void put_unused(QEMUFile *f, void *pv, size_t size)
> +{
> +    fprintf(stderr, "uint8_from_uint32 is used only for backwards 
> compatibility.\n");
> +    fprintf(stderr, "Never should be used to write a new state.\n");
> +    exit(0);
> +}
> +
> +static const VMStateInfo vmstate_hack_uint8_from_uint32 = {
> +    .name = "uint8_from_uint32",
> +    .get  = get_uint8_from_uint32,
> +    .put  = put_unused,
> +};
> +
> +#define VMSTATE_UINT8_HACK_TEST(_f, _s, _t)                           \
> +    VMSTATE_SINGLE_TEST(_f, _s, _t, 0, vmstate_hack_uint8_from_uint32, 
> uint8_t)


So a different format for a different version.
Why is this so bad?
And IMO VMSTATE macros really should be able to cope with
version range checks without open-coded
predicates. These are common and we will have more
of these.


> +
> +static bool between_4_and_8(void *opaque, int version_id)
> +{
> +    return version_id >= 4 && version_id < 8;
> +}
> +

This is pretty ugly, isn't it?
How about ..._V_RANGE() macros?

>  static int virtio_net_post_load(void *opaque, int version_id)
>  {
>      VirtIONet *n = opaque;
> @@ -748,88 +780,37 @@ static int virtio_net_post_load(void *opaque, int 
> version_id)
>      return 0;
>  }
> 
> -static void virtio_net_save(QEMUFile *f, void *opaque)
> -{
> -    VirtIONet *n = opaque;
> -
> -    virtio_save(&n->vdev, f);
> -
> -    qemu_put_buffer(f, n->mac, ETH_ALEN);
> -    qemu_put_be32s(f, &n->tx_timer_active);
> -    qemu_put_be32s(f, &n->mergeable_rx_bufs);
> -    qemu_put_be16s(f, &n->status);
> -    qemu_put_8s(f, &n->promisc);
> -    qemu_put_8s(f, &n->allmulti);
> -    qemu_put_be32s(f, &n->mac_table.in_use);
> -    qemu_put_buffer(f, n->mac_table.macs, n->mac_table.in_use * ETH_ALEN);
> -    qemu_put_buffer(f, n->vlans, MAX_VLAN >> 3);
> -    qemu_put_be32s(f, &n->has_vnet_hdr);
> -    qemu_put_8s(f, &n->mac_table.multi_overflow);
> -    qemu_put_8s(f, &n->mac_table.uni_overflow);
> -    qemu_put_8s(f, &n->alluni);
> -    qemu_put_8s(f, &n->nomulti);
> -    qemu_put_8s(f, &n->nouni);
> -    qemu_put_8s(f, &n->nobcast);
> -    qemu_put_8s(f, &n->has_ufo);
> -}
> -
> -static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
> -{
> -    VirtIONet *n = opaque;
> -
> -    if (version_id < 2 || version_id > VIRTIO_NET_VM_VERSION)
> -        return -EINVAL;
> -
> -    virtio_load(&n->vdev, f);
> -
> -    qemu_get_buffer(f, n->mac, ETH_ALEN);
> -    qemu_get_be32s(f, &n->tx_timer_active);
> -    qemu_get_be32s(f, &n->mergeable_rx_bufs);
> -
> -    if (version_id >= 3)
> -        qemu_get_be16s(f, &n->status);
> -
> -    if (version_id >= 4) {
> -        if (version_id < 8) {
> -            n->promisc = qemu_get_be32(f);
> -            n->allmulti = qemu_get_be32(f);
> -        } else {
> -            qemu_get_8s(f, &n->promisc);
> -            qemu_get_8s(f, &n->allmulti);
> -        }
> -    }
> -
> -    if (version_id >= 5) {
> -        qemu_get_be32s(f, &n->mac_table.in_use);
> -        qemu_get_buffer(f, n->mac_table.macs,
> -                        n->mac_table.in_use * ETH_ALEN);
> -    }
> - 
> -    if (version_id >= 6)
> -        qemu_get_buffer(f, n->vlans, MAX_VLAN >> 3);
> -
> -    if (version_id >= 7) {
> -        qemu_get_be32s(f, &n->has_vnet_hdr);
> -    }
> -
> -    if (version_id >= 9) {
> -        qemu_get_8s(f, &n->mac_table.multi_overflow);
> -        qemu_get_8s(f, &n->mac_table.uni_overflow);
> -    }
> -
> -    if (version_id >= 10) {
> -        qemu_get_8s(f, &n->alluni);
> -        qemu_get_8s(f, &n->nomulti);
> -        qemu_get_8s(f, &n->nouni);
> -        qemu_get_8s(f, &n->nobcast);
> +static const VMStateDescription vmstate_virtio_net = {
> +    .name = "virtio-net",
> +    .version_id = 11,
> +    .minimum_version_id = 2,
> +    .minimum_version_id_old = 2,
> +    .post_load = virtio_net_post_load,
> +    .fields      = (VMStateField []) {
> +        VMSTATE_VIRTIO(vdev, VirtIONet),
> +        VMSTATE_BUFFER(mac, VirtIONet),
> +        VMSTATE_UINT32(tx_timer_active, VirtIONet),
> +        VMSTATE_UINT32(mergeable_rx_bufs, VirtIONet),
> +        VMSTATE_UINT16_V(status, VirtIONet, 3),
> +        VMSTATE_UINT8_HACK_TEST(promisc, VirtIONet, between_4_and_8),
> +        VMSTATE_UINT8_HACK_TEST(allmulti, VirtIONet, between_4_and_8),
> +        VMSTATE_UINT8_V(promisc, VirtIONet, 8),
> +        VMSTATE_UINT8_V(allmulti, VirtIONet, 8),
> +        VMSTATE_UINT32_V(mac_table.in_use, VirtIONet, 5),
> +        VMSTATE_BUFFER_MULTIPLY(mac_table.macs, VirtIONet, 5, NULL, 0,
> +                                mac_table.in_use, ETH_ALEN),
> +        VMSTATE_BUFFER_V(vlans, VirtIONet, 6),
> +        VMSTATE_UINT32_V(has_vnet_hdr, VirtIONet, 7),
> +        VMSTATE_UINT8_V(mac_table.multi_overflow, VirtIONet, 9),
> +        VMSTATE_UINT8_V(mac_table.uni_overflow, VirtIONet, 9),
> +        VMSTATE_UINT8_V(alluni, VirtIONet, 10),
> +        VMSTATE_UINT8_V(nomulti, VirtIONet, 10),
> +        VMSTATE_UINT8_V(nouni, VirtIONet, 10),
> +        VMSTATE_UINT8_V(nobcast, VirtIONet, 10),
> +        VMSTATE_UINT8_V(has_ufo, VirtIONet, 11),
> +        VMSTATE_END_OF_LIST()
>      }
> -
> -    if (version_id >= 11) {
> -        qemu_get_8s(f, &n->has_ufo);
> -    }
> -
> -    return virtio_net_post_load(n, version_id);
> -}
> +};
> 
>  static void virtio_net_cleanup(VLANClientState *vc)
>  {
> @@ -873,8 +854,7 @@ VirtIODevice *virtio_net_init(DeviceState *dev, NICConf 
> *conf)
>      n->mergeable_rx_bufs = 0;
>      n->promisc = 1; /* for compatibility */
> 
> -    register_savevm("virtio-net", virtio_net_id++, VIRTIO_NET_VM_VERSION,
> -                    virtio_net_save, virtio_net_load, n);
> +    vmstate_register(virtio_net_id++, &vmstate_virtio_net, n);
> 
>      return &n->vdev;
>  }
> @@ -885,7 +865,7 @@ void virtio_net_exit(VirtIODevice *vdev)
> 
>      qemu_purge_queued_packets(n->vc);
> 
> -    unregister_savevm("virtio-net", n);
> +    vmstate_unregister(&vmstate_virtio_net, n);
> 
>      qemu_del_timer(n->tx_timer);
>      qemu_free_timer(n->tx_timer);
> -- 
> 1.6.5.2