Re: [PATCH] virtio-net: Fix save/load

[Mark McLoughlin <markmc@redhat.com>]

Hi Alex,

On Thu, 2009-01-15 at 11:05 -0700, Alex Williamson wrote:

> We can't rely on build switches to tell us if a save image
> includes a given field.

Uggh, good point.

> We also need to save status since it's visible to the guest.

Actually, we really need to handle VLANClientState:link_down for all
vlan clients, not just virtio-net and then update status on load
according to link_down.

It's not critically important, though - if we neglect to save/load this
the only side effect is that "set link down" would have to be called
again. Does need to be fixed, though.

>   Draw another line in the sand
> for broken save versions.  The version number should always
> be updated when new values are saved and load should make
> an attempt to set reasonable defaults for lower version save
> images.
> 
> Signed-off-by: Alex Williamson <alex.williamson@hp.com>
> ---
> 
>  qemu/hw/virtio-net.c |   19 +++++++++++++++++--
>  1 files changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/qemu/hw/virtio-net.c b/qemu/hw/virtio-net.c
> index 358c382..6ce2ff5 100644
> --- a/qemu/hw/virtio-net.c
> +++ b/qemu/hw/virtio-net.c
> @@ -21,6 +21,8 @@
>  
>  #define TAP_VNET_HDR
>  
> +#define VIRTIO_NET_VM_VERSION    3
> +
>  typedef struct VirtIONet
>  {
>      VirtIODevice vdev;
> @@ -368,6 +370,11 @@ static void virtio_net_tx_timer(void *opaque)
>      virtio_net_flush_tx(n, n->tx_vq);
>  }
>  
> +/*
> + * Anything added here should cause a bump in VIRTIO_NET_VM_VERSION
> + * and appropriate conditionalized load with sane defaults for older
> + * images should be added to virtio_net_load().
> + */

This is true for all savevm() code, so I don't think we need the comment
here.

>  static void virtio_net_save(QEMUFile *f, void *opaque)
>  {
>      VirtIONet *n = opaque;
> @@ -380,14 +387,18 @@ static void virtio_net_save(QEMUFile *f, void *opaque)
>  
>  #ifdef TAP_VNET_HDR
>      qemu_put_be32(f, tap_has_vnet_hdr(n->vc->vlan->first_client));
> +#else
> +    qemu_put_be32(f, 0);
>  #endif
> +
> +    qemu_put_be16(f, n->status);
>  }
>  
>  static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
>  {
>      VirtIONet *n = opaque;
>  
> -    if (version_id != 2)
> +    if (version_id < 3 || version_id > VIRTIO_NET_VM_VERSION)

This bit isn't right - how can this code load e.g. version 4?
Cheers,
Mark.