[Qemu-devel] [PATCH] vmstate: Avoid seeking

[Qemu-devel] [PATCH] vmstate: Avoid seeking

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2356 bytes --]

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(-)

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 = {


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [Qemu-devel] [PATCH] vmstate: Avoid seeking

[Ryan Harper]

* 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 = {
> 



-- 
Ryan Harper
Software Engineer; Linux Technology Center
IBM Corp., Austin, Tx
ryanh@us.ibm.com

[Qemu-devel] Re: [PATCH] vmstate: Avoid seeking

[Juan Quintela]

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.

[Qemu-devel] Re: [PATCH] vmstate: Avoid seeking

[Jan Kiszka]

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

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

[Qemu-devel] Re: [PATCH] vmstate: Avoid seeking

[Juan Quintela]

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