[Qemu-devel] [PATCH 0/5] migration: Improve subsections detection

[Qemu-devel] [PATCH 0/5] migration: Improve subsections detection

[Juan Quintela]

Hi

v2:
- rename "used" to "remaining" (Alex suggestion)
- implement qemu_get_{byte,buffer} on top of qemu_peek_{byte, buffer}
  (Anthony suggestion)
- fix qemu_peek_buffe_logic (Alex  discovered the problem)

v1:
This series move the subsections detection code form:
- Look that it starts form 5
To:
- Look that it starts form 5 (SUBSECTION)
- Look at the length
- Look that length is bigger than section name
- Look at the idstr and see that it starts with the subsection name.

Please review.

Later, Juan.

Juan Quintela (5):
  savevm: teach qemu_fill_buffer to do partial refills
  savevm: some coding style cleanups
  savevm: define qemu_get_byte() using qemu_peek_byte()
  savevm: improve subsections detection on load
  Revert "savevm: fix corruption in vmstate_subsection_load()."

 savevm.c |  144 ++++++++++++++++++++++++++++++++++++++++---------------------
 1 files changed, 94 insertions(+), 50 deletions(-)

-- 
1.7.6.4

[Qemu-devel] [PATCH 1/5] savevm: teach qemu_fill_buffer to do partial refills

[Juan Quintela]

We will need on next patch to be able to lookahead on next patch

v2: rename "used" to "pending" (Alex Williams)

Signed-off-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
---
 savevm.c |   14 +++++++++++---
 1 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/savevm.c b/savevm.c
index 46f2447..743c304 100644
--- a/savevm.c
+++ b/savevm.c
@@ -455,6 +455,7 @@ void qemu_fflush(QEMUFile *f)
 static void qemu_fill_buffer(QEMUFile *f)
 {
     int len;
+    int pending;

     if (!f->get_buffer)
         return;
@@ -462,10 +463,17 @@ static void qemu_fill_buffer(QEMUFile *f)
     if (f->is_write)
         abort();

-    len = f->get_buffer(f->opaque, f->buf, f->buf_offset, IO_BUF_SIZE);
+    pending = f->buf_size - f->buf_index;
+    if (pending > 0) {
+        memmove(f->buf, f->buf + f->buf_index, pending);
+    }
+    f->buf_index = 0;
+    f->buf_size = pending;
+
+    len = f->get_buffer(f->opaque, f->buf + pending, f->buf_offset,
+                        IO_BUF_SIZE - pending);
     if (len > 0) {
-        f->buf_index = 0;
-        f->buf_size = len;
+        f->buf_size += len;
         f->buf_offset += len;
     } else if (len != -EAGAIN)
         f->has_error = 1;
-- 
1.7.6.4

[Qemu-devel] [PATCH 2/5] savevm: some coding style cleanups

[Juan Quintela]

This patch will make moving code on next patches and having checkpatch
happy easier.

Signed-off-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
---
 savevm.c |   21 ++++++++++++++-------
 1 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/savevm.c b/savevm.c
index 743c304..4069b34 100644
--- a/savevm.c
+++ b/savevm.c
@@ -536,8 +536,9 @@ int qemu_get_buffer(QEMUFile *f, uint8_t *buf, int size1)
 {
     int size, l;

-    if (f->is_write)
+    if (f->is_write) {
         abort();
+    }

     size = size1;
     while (size > 0) {
@@ -545,11 +546,13 @@ int qemu_get_buffer(QEMUFile *f, uint8_t *buf, int size1)
         if (l == 0) {
             qemu_fill_buffer(f);
             l = f->buf_size - f->buf_index;
-            if (l == 0)
+            if (l == 0) {
                 break;
+            }
         }
-        if (l > size)
+        if (l > size) {
             l = size;
+        }
         memcpy(buf, f->buf + f->buf_index, l);
         f->buf_index += l;
         buf += l;
@@ -560,26 +563,30 @@ int qemu_get_buffer(QEMUFile *f, uint8_t *buf, int size1)

 static int qemu_peek_byte(QEMUFile *f)
 {
-    if (f->is_write)
+    if (f->is_write) {
         abort();
+    }

     if (f->buf_index >= f->buf_size) {
         qemu_fill_buffer(f);
-        if (f->buf_index >= f->buf_size)
+        if (f->buf_index >= f->buf_size) {
             return 0;
+        }
     }
     return f->buf[f->buf_index];
 }

 int qemu_get_byte(QEMUFile *f)
 {
-    if (f->is_write)
+    if (f->is_write) {
         abort();
+    }

     if (f->buf_index >= f->buf_size) {
         qemu_fill_buffer(f);
-        if (f->buf_index >= f->buf_size)
+        if (f->buf_index >= f->buf_size) {
             return 0;
+        }
     }
     return f->buf[f->buf_index++];
 }
-- 
1.7.6.4

[Qemu-devel] [PATCH 3/5] savevm: define qemu_get_byte() using qemu_peek_byte()

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 savevm.c |   15 ++++++---------
 1 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/savevm.c b/savevm.c
index 4069b34..94628c6 100644
--- a/savevm.c
+++ b/savevm.c
@@ -578,17 +578,14 @@ static int qemu_peek_byte(QEMUFile *f)

 int qemu_get_byte(QEMUFile *f)
 {
-    if (f->is_write) {
-        abort();
-    }
+    int result;

-    if (f->buf_index >= f->buf_size) {
-        qemu_fill_buffer(f);
-        if (f->buf_index >= f->buf_size) {
-            return 0;
-        }
+    result = qemu_peek_byte(f);
+
+    if (f->buf_index < f->buf_size) {
+        f->buf_index++;
     }
-    return f->buf[f->buf_index++];
+    return result;
 }

 int64_t qemu_ftell(QEMUFile *f)
-- 
1.7.6.4

Re: [Qemu-devel] [PATCH 3/5] savevm: define qemu_get_byte() using qemu_peek_byte()

[Paolo Bonzini]

On 10/06/2011 06:21 PM, Juan Quintela wrote:
> +    result = qemu_peek_byte(f);
> +
> +    if (f->buf_index<  f->buf_size) {
> +        f->buf_index++;
>       }

This should really be an assert that f->buf_index < f->buf_size, 
otherwise qemu_peek_byte has read garbage.

Paolo

Re: [Qemu-devel] [PATCH 3/5] savevm: define qemu_get_byte() using qemu_peek_byte()

[Juan Quintela]

Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 10/06/2011 06:21 PM, Juan Quintela wrote:
>> +    result = qemu_peek_byte(f);
>> +
>> +    if (f->buf_index<  f->buf_size) {
>> +        f->buf_index++;
>>       }
>
> This should really be an assert that f->buf_index < f->buf_size,
> otherwise qemu_peek_byte has read garbage.

That is a change from current behaviour.  qemu_get_byte() returns 0 in
the case that there is nothing to read.  Yes, it is ugly.

Later, Juan.

[Qemu-devel] [PATCH 4/5] savevm: improve subsections detection on load

[Juan Quintela]

We add qemu_peek_buffer, that is identical to qemu_get_buffer, just
that it don't update f->buf_index.

We add a paramenter to qemu_peek_byte() to be able to peek more than
one byte.

Once this is done, to see if we have a subsection we look:
- 1st byte is QEMU_VM_SUBSECTION
- 2nd byte is a length, and is bigger than section name
- 3rd element is a string that starts with section_name

So, we shouldn't have false positives (yes, content could still get us
wrong but probabilities are really low).

v2:
- Alex Williamsom found that we could get negative values on index.
- Rework code to fix that part.
- Rewrite qemu_get_buffer() using qemu_peek_buffer()

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 savevm.c |  110 ++++++++++++++++++++++++++++++++++++++++++--------------------
 1 files changed, 75 insertions(+), 35 deletions(-)

diff --git a/savevm.c b/savevm.c
index 94628c6..28c0a43 100644
--- a/savevm.c
+++ b/savevm.c
@@ -532,59 +532,85 @@ void qemu_put_byte(QEMUFile *f, int v)
         qemu_fflush(f);
 }

-int qemu_get_buffer(QEMUFile *f, uint8_t *buf, int size1)
+static void qemu_file_skip(QEMUFile *f, int size)
 {
-    int size, l;
+    if (f->buf_index + size < f->buf_size) {
+        f->buf_index += size;
+    }
+}
+
+static int qemu_peek_buffer(QEMUFile *f, uint8_t *buf, int size, size_t offset)
+{
+    int pending;
+    int index;

     if (f->is_write) {
         abort();
     }

-    size = size1;
-    while (size > 0) {
-        l = f->buf_size - f->buf_index;
-        if (l == 0) {
-            qemu_fill_buffer(f);
-            l = f->buf_size - f->buf_index;
-            if (l == 0) {
-                break;
-            }
-        }
-        if (l > size) {
-            l = size;
+    index = f->buf_index + offset;
+    pending = f->buf_size - index;
+    if (pending < size) {
+        qemu_fill_buffer(f);
+        index = f->buf_index + offset;
+        pending = f->buf_size - index;
+    }
+
+    if (pending <= 0) {
+        return 0;
+    }
+    if (size > pending) {
+        size = pending;
+    }
+
+    memcpy(buf, f->buf + index, size);
+    return size;
+}
+
+int qemu_get_buffer(QEMUFile *f, uint8_t *buf, int size)
+{
+    int pending = size;
+    int done = 0;
+
+    while (pending > 0) {
+        int res;
+
+        res = qemu_peek_buffer(f, buf, pending, 0);
+        if (res == 0) {
+            return 0;
         }
-        memcpy(buf, f->buf + f->buf_index, l);
-        f->buf_index += l;
-        buf += l;
-        size -= l;
+        qemu_file_skip(f, res);
+        buf += res;
+        pending -= res;
+        done += res;
     }
-    return size1 - size;
+    return done;
 }

-static int qemu_peek_byte(QEMUFile *f)
+static int qemu_peek_byte(QEMUFile *f, int offset)
 {
+    int index = f->buf_index + offset;
+
     if (f->is_write) {
         abort();
     }

-    if (f->buf_index >= f->buf_size) {
+    if (index >= f->buf_size) {
         qemu_fill_buffer(f);
-        if (f->buf_index >= f->buf_size) {
+        index = f->buf_index + offset;
+        if (index >= f->buf_size) {
             return 0;
         }
     }
-    return f->buf[f->buf_index];
+    return f->buf[index];
 }

 int qemu_get_byte(QEMUFile *f)
 {
     int result;

-    result = qemu_peek_byte(f);
-
-    if (f->buf_index < f->buf_size) {
-        f->buf_index++;
-    }
+    result = qemu_peek_byte(f, 0);
+    qemu_file_skip(f, 1);
     return result;
 }

@@ -1684,22 +1710,36 @@ static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
         return 0;
     }

-    while (qemu_peek_byte(f) == QEMU_VM_SUBSECTION) {
+    while (qemu_peek_byte(f, 0) == QEMU_VM_SUBSECTION) {
         char idstr[256];
         int ret;
-        uint8_t version_id, len;
+        uint8_t version_id, len, size;
         const VMStateDescription *sub_vmsd;

-        qemu_get_byte(f); /* subsection */
-        len = qemu_get_byte(f);
-        qemu_get_buffer(f, (uint8_t *)idstr, len);
-        idstr[len] = 0;
-        version_id = qemu_get_be32(f);
+        len = qemu_peek_byte(f, 1);
+        if (len < strlen(vmsd->name) + 1) {
+            /* subsection name has be be "section_name/a" */
+            return 0;
+        }
+        size = qemu_peek_buffer(f, (uint8_t *)idstr, len, 2);
+        if (size != len) {
+            return 0;
+        }
+        idstr[size] = 0;

+        if (strncmp(vmsd->name, idstr, strlen(vmsd->name)) != 0) {
+            /* it don't have a valid subsection name */
+            return 0;
+        }
         sub_vmsd = vmstate_get_subsection(sub, idstr);
         if (sub_vmsd == NULL) {
             return -ENOENT;
         }
+        qemu_file_skip(f, 1); /* subsection */
+        qemu_file_skip(f, 1); /* len */
+        qemu_file_skip(f, len); /* idstr */
+        version_id = qemu_get_be32(f);
+
         assert(!sub_vmsd->subsections);
         ret = vmstate_load_state(f, sub_vmsd, opaque, version_id);
         if (ret) {
-- 
1.7.6.4

Re: [Qemu-devel] [PATCH 4/5] savevm: improve subsections detection on load

[Paolo Bonzini]

On 10/06/2011 06:21 PM, Juan Quintela wrote:
> +
> +int qemu_get_buffer(QEMUFile *f, uint8_t *buf, int size)
> +{
> +    int pending = size;
> +    int done = 0;
> +
> +    while (pending>  0) {
> +        int res;
> +
> +        res = qemu_peek_buffer(f, buf, pending, 0);
> +        if (res == 0) {
> +            return 0;
>           }
> -        memcpy(buf, f->buf + f->buf_index, l);
> -        f->buf_index += l;
> -        buf += l;
> -        size -= l;
> +        qemu_file_skip(f, res);
> +        buf += res;
> +        pending -= res;
> +        done += res;
>       }
> -    return size1 - size;
> +    return done;
>   }

This changes semantics for reads above 32KB.  It should be in the commit 
message, or preferably v1 could be committed instead. :)

Paolo

Re: [Qemu-devel] [PATCH 4/5] savevm: improve subsections detection on load

[Juan Quintela]

Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 10/06/2011 06:21 PM, Juan Quintela wrote:
>> +
>> +int qemu_get_buffer(QEMUFile *f, uint8_t *buf, int size)
>> +{
>> +    int pending = size;
>> +    int done = 0;
>> +
>> +    while (pending>  0) {
>> +        int res;
>> +
>> +        res = qemu_peek_buffer(f, buf, pending, 0);
>> +        if (res == 0) {
>> +            return 0;

should this line return "done" insntead?

>>           }
>> -        memcpy(buf, f->buf + f->buf_index, l);
>> -        f->buf_index += l;
>> -        buf += l;
>> -        size -= l;
>> +        qemu_file_skip(f, res);
>> +        buf += res;
>> +        pending -= res;
>> +        done += res;
>>       }
>> -    return size1 - size;
>> +    return done;
>>   }
>
> This changes semantics for reads above 32KB.  It should be in the
> commit message, or preferably v1 could be committed instead. :)

how it changes?  My understanding is that we read the same, only change
that I can think of is the one that I have jsut shown (and that is on
the error case).

Later, Juan.

Re: [Qemu-devel] [PATCH 4/5] savevm: improve subsections detection on load

[Paolo Bonzini]

On 10/07/2011 12:42 AM, Juan Quintela wrote:
>> >  This changes semantics for reads above 32KB.  It should be in the
>> >  commit message, or preferably v1 could be committed instead.:)
> how it changes?  My understanding is that we read the same, only change
> that I can think of is the one that I have jsut shown (and that is on
> the error case).

Yes, you're right.

Paolo

[Qemu-devel] [PATCH 5/5] Revert "savevm: fix corruption in vmstate_subsection_load()."

[Juan Quintela]

This reverts commit eb60260de0b050a5e8ab725e84d377d0b44c43ae.

Conflicts:

	savevm.c

We changed qemu_peek_byte() prototype, just fixed the rejects.

Signed-off-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
---
 savevm.c |   10 +---------
 1 files changed, 1 insertions(+), 9 deletions(-)

diff --git a/savevm.c b/savevm.c
index 28c0a43..1c62269 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1704,12 +1704,6 @@ static const VMStateDescription *vmstate_get_subsection(const VMStateSubsection
 static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
                                    void *opaque)
 {
-    const VMStateSubsection *sub = vmsd->subsections;
-
-    if (!sub || !sub->needed) {
-        return 0;
-    }
-
     while (qemu_peek_byte(f, 0) == QEMU_VM_SUBSECTION) {
         char idstr[256];
         int ret;
@@ -1731,7 +1725,7 @@ static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
             /* it don't have a valid subsection name */
             return 0;
         }
-        sub_vmsd = vmstate_get_subsection(sub, idstr);
+        sub_vmsd = vmstate_get_subsection(vmsd->subsections, idstr);
         if (sub_vmsd == NULL) {
             return -ENOENT;
         }
@@ -1740,7 +1734,6 @@ static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
         qemu_file_skip(f, len); /* idstr */
         version_id = qemu_get_be32(f);

-        assert(!sub_vmsd->subsections);
         ret = vmstate_load_state(f, sub_vmsd, opaque, version_id);
         if (ret) {
             return ret;
@@ -1764,7 +1757,6 @@ static void vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd,
             qemu_put_byte(f, len);
             qemu_put_buffer(f, (uint8_t *)vmsd->name, len);
             qemu_put_be32(f, vmsd->version_id);
-            assert(!vmsd->subsections);
             vmstate_save_state(f, vmsd, opaque);
         }
         sub++;
-- 
1.7.6.4

[Qemu-devel] [PATCH v3 0/5] migration: Improve subsections detection

[Juan Quintela]

Hi

v3:
- fix return value on qemu_get_buffer.

Anthony, all reviewers comments are fixed, please consider to apply.

Later, Juan.

v2:
- rename "used" to "remaining" (Alex suggestion)
- implement qemu_get_{byte,buffer} on top of qemu_peek_{byte, buffer}
  (Anthony suggestion)
- fix qemu_peek_buffe_logic (Alex  discovered the problem)

v1:
This series move the subsections detection code form:
- Look that it starts form 5
To:
- Look that it starts form 5 (SUBSECTION)
- Look at the length
- Look that length is bigger than section name
- Look at the idstr and see that it starts with the subsection name.

Please review.

Later, Juan.

Juan Quintela (5):
  savevm: teach qemu_fill_buffer to do partial refills
  savevm: some coding style cleanups
  savevm: define qemu_get_byte() using qemu_peek_byte()
  savevm: improve subsections detection on load
  Revert "savevm: fix corruption in vmstate_subsection_load()."

 savevm.c |  144 ++++++++++++++++++++++++++++++++++++++++---------------------
 1 files changed, 94 insertions(+), 50 deletions(-)

-- 
1.7.6.4

[Qemu-devel] [PATCH 3/5] savevm: define qemu_get_byte() using qemu_peek_byte()

[Juan Quintela]

Signed-off-by: Juan Quintela<quintela@redhat.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 savevm.c |   15 ++++++---------
 1 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/savevm.c b/savevm.c
index 4069b34..94628c6 100644
--- a/savevm.c
+++ b/savevm.c
@@ -578,17 +578,14 @@ static int qemu_peek_byte(QEMUFile *f)

 int qemu_get_byte(QEMUFile *f)
 {
-    if (f->is_write) {
-        abort();
-    }
+    int result;

-    if (f->buf_index >= f->buf_size) {
-        qemu_fill_buffer(f);
-        if (f->buf_index >= f->buf_size) {
-            return 0;
-        }
+    result = qemu_peek_byte(f);
+
+    if (f->buf_index < f->buf_size) {
+        f->buf_index++;
     }
-    return f->buf[f->buf_index++];
+    return result;
 }

 int64_t qemu_ftell(QEMUFile *f)
-- 
1.7.6.4

[Qemu-devel] [PATCH v4 0/5] Improve subsections detection

[Juan Quintela]

Hi

v4:
- brown paperbug on qemu_file_skip()

static void qemu_file_skip(QEMUFile *f, int size)
{
-    if (f->buf_index + size < f->buf_size) {
+    if (f->buf_index + size <= f->buf_size) {
        f->buf_index += size;
    }
}

v3:
- fix return value on qemu_get_buffer.

Anthony, all reviewers comments are fixed, please consider to apply.

Later, Juan.

v2:
- rename "used" to "remaining" (Alex suggestion)
- implement qemu_get_{byte,buffer} on top of qemu_peek_{byte, buffer}
  (Anthony suggestion)
- fix qemu_peek_buffe_logic (Alex  discovered the problem)

v1:
This series move the subsections detection code form:
- Look that it starts form 5
To:
- Look that it starts form 5 (SUBSECTION)
- Look at the length
- Look that length is bigger than section name
- Look at the idstr and see that it starts with the subsection name.

Please review.

Later, Juan.


*** BLURB HERE ***

Juan Quintela (5):
  savevm: teach qemu_fill_buffer to do partial refills
  savevm: some coding style cleanups
  savevm: define qemu_get_byte() using qemu_peek_byte()
  savevm: improve subsections detection on load
  Revert "savevm: fix corruption in vmstate_subsection_load()."

 savevm.c |  144 ++++++++++++++++++++++++++++++++++++++++---------------------
 1 files changed, 94 insertions(+), 50 deletions(-)

-- 
1.7.6.4

[Qemu-devel] [PATCH 3/5] savevm: define qemu_get_byte() using qemu_peek_byte()

[Juan Quintela]

Signed-off-by: Juan Quintela<quintela@redhat.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 savevm.c |   15 ++++++---------
 1 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/savevm.c b/savevm.c
index 4069b34..94628c6 100644
--- a/savevm.c
+++ b/savevm.c
@@ -578,17 +578,14 @@ static int qemu_peek_byte(QEMUFile *f)

 int qemu_get_byte(QEMUFile *f)
 {
-    if (f->is_write) {
-        abort();
-    }
+    int result;

-    if (f->buf_index >= f->buf_size) {
-        qemu_fill_buffer(f);
-        if (f->buf_index >= f->buf_size) {
-            return 0;
-        }
+    result = qemu_peek_byte(f);
+
+    if (f->buf_index < f->buf_size) {
+        f->buf_index++;
     }
-    return f->buf[f->buf_index++];
+    return result;
 }

 int64_t qemu_ftell(QEMUFile *f)
-- 
1.7.6.4