| Message ID | 4B15A8FA.5080804@web.de |
|---|---|
| State | New |
| Headers | show |
* Jan Kiszka <jan.kiszka@web.de> [2009-12-01 17:44]: > Seeking on vmstate save/load does not work if the underlying file is a > stream. We could try to make all QEMUFile* forward-seek-aware, but first > attempts in this direction indicated that it's saner to convert the few > qemu_fseek-on-vmstates users to plain reads/writes. > > This fixes various subtle vmstate corruptions where unused fields were > involved. > > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> With this patch and the VMSTATE_MACADDR patch, localhost migration with e1000 is working again. Acked-by: Ryan Harper <ryanh@us.ibm.com> > --- > > hw/virtio-net.c | 7 ++----- > savevm.c | 18 ++++++++++++++++-- > 2 files changed, 18 insertions(+), 7 deletions(-) > > diff --git a/hw/virtio-net.c b/hw/virtio-net.c > index 2f147e5..9ccd4c8 100644 > --- a/hw/virtio-net.c > +++ b/hw/virtio-net.c > @@ -745,12 +745,9 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id) > > if (version_id >= 5) { > n->mac_table.in_use = qemu_get_be32(f); > + qemu_get_buffer(f, n->mac_table.macs, n->mac_table.in_use * ETH_ALEN); > /* MAC_TABLE_ENTRIES may be different from the saved image */ > - if (n->mac_table.in_use <= MAC_TABLE_ENTRIES) { > - qemu_get_buffer(f, n->mac_table.macs, > - n->mac_table.in_use * ETH_ALEN); > - } else if (n->mac_table.in_use) { > - qemu_fseek(f, n->mac_table.in_use * ETH_ALEN, SEEK_CUR); > + if (n->mac_table.in_use > MAC_TABLE_ENTRIES) { > n->mac_table.multi_overflow = n->mac_table.uni_overflow = 1; > n->mac_table.in_use = 0; > } > diff --git a/savevm.c b/savevm.c > index 8fe9349..1e54a42 100644 > --- a/savevm.c > +++ b/savevm.c > @@ -959,13 +959,27 @@ const VMStateInfo vmstate_info_buffer = { > > static int get_unused_buffer(QEMUFile *f, void *pv, size_t size) > { > - qemu_fseek(f, size, SEEK_CUR); > + uint8_t buf[1024]; > + int block_len; > + > + while (size > 0) { > + block_len = MIN(sizeof(buf), size); > + size -= block_len; > + qemu_get_buffer(f, buf, block_len); > + } > return 0; > } > > static void put_unused_buffer(QEMUFile *f, void *pv, size_t size) > { > - qemu_fseek(f, size, SEEK_CUR); > + static const uint8_t buf[1024]; > + int block_len; > + > + while (size > 0) { > + block_len = MIN(sizeof(buf), size); > + size -= block_len; > + qemu_put_buffer(f, buf, block_len); > + } > } > > const VMStateInfo vmstate_info_unused_buffer = { >
Jan Kiszka <jan.kiszka@web.de> wrote: > Seeking on vmstate save/load does not work if the underlying file is a > stream. We could try to make all QEMUFile* forward-seek-aware, but first > attempts in this direction indicated that it's saner to convert the few > qemu_fseek-on-vmstates users to plain reads/writes. > > This fixes various subtle vmstate corruptions where unused fields were > involved. > > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> Something changed lately. This used to work, and I also waste^spend yesterday trying to understand why it was failing to me. I am splitting the patch in virtio-net and savevm parts. (In my tree virtio-net don't use fseek anymore). Thanks for finding the bug. Later, Juan.
Juan Quintela wrote: > Jan Kiszka <jan.kiszka@web.de> wrote: >> Seeking on vmstate save/load does not work if the underlying file is a >> stream. We could try to make all QEMUFile* forward-seek-aware, but first >> attempts in this direction indicated that it's saner to convert the few >> qemu_fseek-on-vmstates users to plain reads/writes. >> >> This fixes various subtle vmstate corruptions where unused fields were >> involved. >> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> > > Something changed lately. This used to work, and I also waste^spend > yesterday trying to understand why it was failing to me. I'm quite sure it never really worked. Maybe the bug was just papered over. > > I am splitting the patch in virtio-net and savevm parts. (In my tree > virtio-net don't use fseek anymore). OK, then I will drop this patch from my queue. BTW, where is your tree hosted? > > Thanks for finding the bug. > > Later, Juan. Jan
Jan Kiszka <jan.kiszka@siemens.com> wrote: > Juan Quintela wrote: >> Jan Kiszka <jan.kiszka@web.de> wrote: >>> Seeking on vmstate save/load does not work if the underlying file is a >>> stream. We could try to make all QEMUFile* forward-seek-aware, but first >>> attempts in this direction indicated that it's saner to convert the few >>> qemu_fseek-on-vmstates users to plain reads/writes. >>> >>> This fixes various subtle vmstate corruptions where unused fields were >>> involved. >>> >>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> >> >> Something changed lately. This used to work, and I also waste^spend >> yesterday trying to understand why it was failing to me. > > I'm quite sure it never really worked. Maybe the bug was just papered over. > >> >> I am splitting the patch in virtio-net and savevm parts. (In my tree >> virtio-net don't use fseek anymore). > > OK, then I will drop this patch from my queue. BTW, where is your tree > hosted? http://repo.or.cz/w/qemu/quintela.git/shortlog/refs/heads/vmstate/virtio This one has not still in upstream: - the audio changes that I have just posted - vmstate cleanups (yours and more) - msix/virtio port to vmstate I am splitting vmstate/cleanups at this moment to send the two series upstream. I am in the point where everything compiles and run, i.e. only need to reorder patches. Later, Juan. >> >> Thanks for finding the bug. >> >> Later, Juan. > > Jan
diff --git a/hw/virtio-net.c b/hw/virtio-net.c index 2f147e5..9ccd4c8 100644 --- a/hw/virtio-net.c +++ b/hw/virtio-net.c @@ -745,12 +745,9 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id) if (version_id >= 5) { n->mac_table.in_use = qemu_get_be32(f); + qemu_get_buffer(f, n->mac_table.macs, n->mac_table.in_use * ETH_ALEN); /* MAC_TABLE_ENTRIES may be different from the saved image */ - if (n->mac_table.in_use <= MAC_TABLE_ENTRIES) { - qemu_get_buffer(f, n->mac_table.macs, - n->mac_table.in_use * ETH_ALEN); - } else if (n->mac_table.in_use) { - qemu_fseek(f, n->mac_table.in_use * ETH_ALEN, SEEK_CUR); + if (n->mac_table.in_use > MAC_TABLE_ENTRIES) { n->mac_table.multi_overflow = n->mac_table.uni_overflow = 1; n->mac_table.in_use = 0; } diff --git a/savevm.c b/savevm.c index 8fe9349..1e54a42 100644 --- a/savevm.c +++ b/savevm.c @@ -959,13 +959,27 @@ const VMStateInfo vmstate_info_buffer = { static int get_unused_buffer(QEMUFile *f, void *pv, size_t size) { - qemu_fseek(f, size, SEEK_CUR); + uint8_t buf[1024]; + int block_len; + + while (size > 0) { + block_len = MIN(sizeof(buf), size); + size -= block_len; + qemu_get_buffer(f, buf, block_len); + } return 0; } static void put_unused_buffer(QEMUFile *f, void *pv, size_t size) { - qemu_fseek(f, size, SEEK_CUR); + static const uint8_t buf[1024]; + int block_len; + + while (size > 0) { + block_len = MIN(sizeof(buf), size); + size -= block_len; + qemu_put_buffer(f, buf, block_len); + } } const VMStateInfo vmstate_info_unused_buffer = {
Seeking on vmstate save/load does not work if the underlying file is a stream. We could try to make all QEMUFile* forward-seek-aware, but first attempts in this direction indicated that it's saner to convert the few qemu_fseek-on-vmstates users to plain reads/writes. This fixes various subtle vmstate corruptions where unused fields were involved. Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> --- hw/virtio-net.c | 7 ++----- savevm.c | 18 ++++++++++++++++-- 2 files changed, 18 insertions(+), 7 deletions(-)