| Message ID | bcca3343e5ec8fe6c2d011c87d496426f9ec5a5f.1259754427.git.quintela@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Dec 02, 2009 at 01:04:31PM +0100, Juan Quintela wrote: > > Signed-off-by: Juan Quintela <quintela@redhat.com> > --- > 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. > + */ > + > +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"); line too long > + fprintf(stderr, "Never should be used to write a new state.\n"); > + exit(0); I don't understand. what is this dong? when is this called? Please supply a comment. Maybe call assert? > +} > + > +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) > + > +static bool between_4_and_8(void *opaque, int version_id) > +{ > + return version_id >= 4 && version_id < 8; > +} > + > 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
On Wed, Dec 02, 2009 at 01:04:31PM +0100, Juan Quintela wrote: > > Signed-off-by: Juan Quintela <quintela@redhat.com> > --- > 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
"Michael S. Tsirkin" <mst@redhat.com> wrote: > On Wed, Dec 02, 2009 at 01:04:31PM +0100, Juan Quintela wrote: >> >> Signed-off-by: Juan Quintela <quintela@redhat.com> >> --- >> 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. >> + */ >> + >> +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"); > > line too long > >> + fprintf(stderr, "Never should be used to write a new state.\n"); >> + exit(0); > > I don't understand. what is this dong? it is used later. current code is reading an uint32_t value into a int8_t value. As you can guess that don't fit (that is the HACK part of it). That is only needed for old versions that we are reading (get_* function has real code). But we are supposed to never write old versions (*). Thet that shouldn't happen ever. > when > is this called? Please supply a comment. > Maybe call assert? > assert or exit is ok for me, what does people preffer? Later, Juan. (*): My next series will propose to change that and allow to write old versions, but that didn't exist when this code was written, and there are still no agreement about how/if doing it.
On Wed, Dec 02, 2009 at 07:38:03PM +0100, Juan Quintela wrote: > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > On Wed, Dec 02, 2009 at 01:04:31PM +0100, Juan Quintela wrote: > >> > >> Signed-off-by: Juan Quintela <quintela@redhat.com> > >> --- > >> 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. > >> + */ > >> + > >> +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"); > > > > line too long > > > >> + fprintf(stderr, "Never should be used to write a new state.\n"); > >> + exit(0); > > > > I don't understand. what is this dong? > > it is used later. > current code is reading an uint32_t value into a int8_t value. As you > can guess that don't fit (that is the HACK part of it). I expect it fits in practice. But you should range check the value and fail migration on error. > That is only > needed for old versions that we are reading (get_* function has real > code). But we are supposed to never write old versions (*). > Thet that shouldn't happen ever. vmstate guarantees this won't be called? So just assert(1)? Let's not write a ton of code that isn't called? > > > > when > > is this called? Please supply a comment. > > Maybe call assert? > > > > assert or exit is ok for me, what does people preffer? > > Later, Juan. > > (*): My next series will propose to change that and allow to write old > versions, but that didn't exist when this code was written, and > there are still no agreement about how/if doing it.
"Michael S. Tsirkin" <mst@redhat.com> wrote: > On Wed, Dec 02, 2009 at 07:38:03PM +0100, Juan Quintela wrote: >> "Michael S. Tsirkin" <mst@redhat.com> wrote: > I expect it fits in practice. > But you should range check the value and fail migration on error. > >> That is only >> needed for old versions that we are reading (get_* function has real >> code). But we are supposed to never write old versions (*). >> Thet that shouldn't happen ever. > > vmstate guarantees this won't be called? No, I express it badly. That function should never be called. It is a compatibility type for old versions. Only get() part should be used. What should be guaranteed is that you protect that with a test or something to assure that only the get() part is ever used, not the put() one. > So just assert(1)? Let's not write a ton of > code that isn't called? options are: a- put a NULL value in the struct, and if there are ever an error get a segmentation fault. b- put a function that just writes "the impossible happened in <foo>" and exit. I don't know where do you want me to put one assert. Later, Juan. >> > when >> > is this called? Please supply a comment. >> > Maybe call assert? >> > >> >> assert or exit is ok for me, what does people preffer? >> >> Later, Juan. >> >> (*): My next series will propose to change that and allow to write old >> versions, but that didn't exist when this code was written, and >> there are still no agreement about how/if doing it.
"Michael S. Tsirkin" <mst@redhat.com> wrote: > On Wed, Dec 02, 2009 at 01:04:31PM +0100, Juan Quintela wrote: >> >> Signed-off-by: Juan Quintela <quintela@redhat.com> >> --- >> 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). but u32 don't fit in u8. And we read u32 into u8. I agree that writing u8 into u32 is not a problem, the problem is the reverse. > IMO we should teach infrastructure to cope > wit this gracefully without littering > code with _hack suffixes. There is no way to get this one. vmstate only knows about types. You want vmstate to know that: - sometimes writing u32 in u8 is one error - othertimes writing u32 in u8 is correct, because we should have using u8 in the 1st place. vmstate only has to look at: type of field and function used to send/receive field. No code analisys to know how many values are used of that type, etc. What code is doing here is not valid in general. It is valid in this particular case, that is the reason why it is a _hack. Later, Juan. >> + >> +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. if they are common, we can have a library of them. That is not the problem. cope with version range checks is not so trivial (at least we need to add another field to store the in,from). And there are places where that is not enough (think of the virtio check to know if it is pci/msix). >> + >> +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? I am really thinking about remove the _V() version, and move all to tests. reason for that is that there are already too much arguments that are integers. If we add more, we end having something like: VMSTATE_FOO(bar, FOOState, 1, 2, 3, 4) Once that we are there, moving one integer to the wrong place is too easy. In the other hand, it is trivial to have already defined functions that are: v1 v2 v3 v1_8 .... And for that, I can typecheck that you are not putting integers in the place of versions. technically, there is easy to add _RANGE() macros. My problem is that each time that we add another integer parameter, we make the interfaz more difficult to use. This proposal has the advantage that it removes the version parameter and improves typechecking. Later, Juan.
On Wed, Dec 02, 2009 at 08:18:40PM +0100, Juan Quintela wrote: > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > On Wed, Dec 02, 2009 at 01:04:31PM +0100, Juan Quintela wrote: > >> > >> Signed-off-by: Juan Quintela <quintela@redhat.com> > >> --- > >> 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). > > but u32 don't fit in u8. And we read u32 into u8. I agree that writing > u8 into u32 is not a problem, the problem is the reverse. So don't read u32 into u8. Read data into u32, check that the value fits into u8, fail migration if it does not, assign value if it does. > > IMO we should teach infrastructure to cope > > wit this gracefully without littering > > code with _hack suffixes. > > There is no way to get this one. vmstate only knows about types. > You want vmstate to know that: > - sometimes writing u32 in u8 is one error > - othertimes writing u32 in u8 is correct, because we should have using > u8 in the 1st place. > > vmstate only has to look at: > type of field and function used to send/receive field. No code analisys > to know how many values are used of that type, etc. > > What code is doing here is not valid in general. It is valid in this > particular case, that is the reason why it is a _hack. > > Later, Juan. Assume that you want to make format forward compatible. If you want to support large values in the future, you reserve 4 bytes in the format. This makes more sense than playing with versions, and in my book, this is a good practice, not a hack. This should also not force you to store values at runtime as a 32 bit integers. > >> + > >> +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. > > if they are common, we can have a library of them. That is not the > problem. cope with version range checks is not so trivial (at least we > need to add another field to store the in,from). And there are places > where that is not enough (think of the virtio check to know if it is > pci/msix). > > >> + > >> +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? > > I am really thinking about remove the _V() version, and move all to > tests. reason for that is that there are already too much arguments > that are integers. If we add more, we end having something like: > > VMSTATE_FOO(bar, FOOState, 1, 2, 3, 4) > > Once that we are there, moving one integer to the wrong place is too > easy. In the other hand, it is trivial to have already defined > functions that are: > v1 > v2 > v3 > v1_8 > .... > And for that, I can typecheck that you are not putting integers in the > place of versions. > > technically, there is easy to add _RANGE() macros. My problem is that > each time that we add another integer parameter, we make the interfaz > more difficult to use. I agree this is a problem. Another option is to use structures initializing any extra fields outside the macro. { .min_version = 1, .max_version = 2, VMSTATE_FOO(bar, FOOState), }, > > This proposal has the advantage that it removes the version parameter > and improves typechecking. > > Later, Juan.
"Michael S. Tsirkin" <mst@redhat.com> wrote: ... >> but u32 don't fit in u8. And we read u32 into u8. I agree that writing >> u8 into u32 is not a problem, the problem is the reverse. > > So don't read u32 into u8. > Read data into u32, check that the value fits into u8, fail migration > if it does not, assign value if it does. Exactly what the code does. That is one of the definitions of "hack" :) >> > IMO we should teach infrastructure to cope >> > wit this gracefully without littering >> > code with _hack suffixes. >> >> There is no way to get this one. vmstate only knows about types. >> You want vmstate to know that: >> - sometimes writing u32 in u8 is one error >> - othertimes writing u32 in u8 is correct, because we should have using >> u8 in the 1st place. >> >> vmstate only has to look at: >> type of field and function used to send/receive field. No code analisys >> to know how many values are used of that type, etc. >> >> What code is doing here is not valid in general. It is valid in this >> particular case, that is the reason why it is a _hack. >> >> Later, Juan. > > Assume that you want to make format forward compatible. If you want to > support large values in the future, you reserve 4 bytes in the format. > This makes more sense than playing with versions, and in my book, this > is a good practice, not a hack. This should also not force you to store > values at runtime as a 32 bit integers. If you want 32bits, use 32bits. No tricks, no games, all right. If you want to read 8bit into 32 bits, that is doable. If you want to store 32bits into 8bit -> unsafe in general. If you want to do that, you get a big RED sing, you can call it "UNSAFE", "HACK", whatever. Typechecking is lost. >> Once that we are there, moving one integer to the wrong place is too >> easy. In the other hand, it is trivial to have already defined >> functions that are: >> v1 >> v2 >> v3 >> v1_8 >> .... >> And for that, I can typecheck that you are not putting integers in the >> place of versions. >> >> technically, there is easy to add _RANGE() macros. My problem is that >> each time that we add another integer parameter, we make the interfaz >> more difficult to use. > > I agree this is a problem. Another option is to use structures > initializing any extra fields outside the macro. > > { > .min_version = 1, > .max_version = 2, > VMSTATE_FOO(bar, FOOState), > }, That is another option. I would preffer the test functions, because as we need them anyways, that makes things easier. vmstate_field was pretty small at the beggining, now it is starting to bloat. Later, Juan.
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. + */ + +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) + +static bool between_4_and_8(void *opaque, int version_id) +{ + return version_id >= 4 && version_id < 8; +} + 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);
Signed-off-by: Juan Quintela <quintela@redhat.com> --- hw/virtio-net.c | 148 ++++++++++++++++++++++++------------------------------- 1 files changed, 64 insertions(+), 84 deletions(-)