Make qemu_peek_buffer loop until it gets it's data

?? 2014??4??8????????10:29??Dr. David Alan Gilbert (git) <[email protected]> ??????

Quoted 5 lines

From: "Dr. David Alan Gilbert" <[email protected]>

Make qemu_peek_buffer repeatedly call fill_buffer until it gets all the data it requires, or until there is an error.

Quoted 6 lines

At the moment, qemu_peek_buffer will try one qemu_fill_buffer if there isn't enough data waiting, however the kernel is entitled to return just a few bytes, and still leave qemu_peek_buffer with less bytes than it needed. I've seen this fail in a dev world, and I think it could theoretically fail in the peeking of the subsection headers in the current world.

hmm, I also have got some errors(infrequently). Maybe It is one point. Could you show some messages about the error?

However, this patch avoids one potential issue, thanks.

Reviewed-by ChenLiang <[email protected]>

Quoted 147 lines

Comment qemu_peek_byte to point out it's not guaranteed to work for
non-continuous peeks

Signed-off-by Dr. David Alan Gilbert <[email protected]>

---

v4:
Limit the size qemu_get_buffer passes to qemu_fill_buffer on each loop

v3:
Make qemu_fill_buffer return ssize_t
Remove the other size_t cleanup from this patch - I'll get back to them later Comment additions/cleanups from review
Stretch my -ve

include/migration/qemu-file.h | 5 ++++
qemu-file.c | 53 +++++++++++++++++++++++++++++++++++++++----

2 files changed, 54 insertions(+), 4 deletions(-)

include/migration/qemu-file.h +5 -0

index a191fb6..c90f529 100644
@@ -123,6 +123,11 @@ void qemu_put_be32(QEMUFile *f, unsigned int v);
 void qemu_put_be64(QEMUFile *f, uint64_t v);
 int qemu_peek_buffer(QEMUFile *f, uint8_t *buf, int size, size_t offset);
 int qemu_get_buffer(QEMUFile *f, uint8_t *buf, int size);
+/*
+ * Note that you can only peek continuous bytes from where the current pointer
+ * is; you aren't guaranteed to be able to peak to +n bytes unless you've
+ * previously peeked +n-1.
+ */
 int qemu_peek_byte(QEMUFile *f, int offset);
 int qemu_get_byte(QEMUFile *f);
 void qemu_file_skip(QEMUFile *f, int size);

qemu-file.c +49 -4

index e5ec798..6ae37d0 100644
@@ -529,7 +529,15 @@ size_t ram_control_save_page(QEMUFile *f, ram_addr_t block_offset,
    return RAM_SAVE_CONTROL_NOT_SUPP;
 }
 
-static void qemu_fill_buffer(QEMUFile *f)
+/*
+ * Attempt to fill the buffer from the underlying file
+ * Returns the number of bytes read, or negative value for an error.
+ *
+ * Note that it can return a partially full buffer even in a not error/not EOF
+ * case if the underlying file descriptor gives a short read, and that can
+ * happen even on a blocking fd.
+ */
+static ssize_t qemu_fill_buffer(QEMUFile *f)
 {
    int len;
    int pending;

@@ -553,6 +561,8 @@ static void qemu_fill_buffer(QEMUFile *f)
    } else if (len != -EAGAIN) {
        qemu_file_set_error(f, len);
    }
+
+    return len;
 }
 
 int qemu_get_fd(QEMUFile *f)

@@ -683,17 +693,39 @@ void qemu_file_skip(QEMUFile *f, int size)
    }
 }
 
+/*
+ * Read 'size' bytes from file (at 'offset') into buf without moving the
+ * pointer.
+ *
+ * It will return size bytes unless there was an error, in which case it will
+ * return as many as it managed to read (assuming blocking fd's which
+ * all current QEMUFile are)
+ */
 int qemu_peek_buffer(QEMUFile *f, uint8_t *buf, int size, size_t offset)
 {
    int pending;
    int index;
 
    assert(!qemu_file_is_writable(f));
+    assert(offset < IO_BUF_SIZE);
+    assert(size <= IO_BUF_SIZE - offset);
 
+    /* The 1st byte to read from */
    index = f->buf_index + offset;
+    /* The number of available bytes starting at index */
    pending = f->buf_size - index;
-    if (pending < size) {
-        qemu_fill_buffer(f);
+
+    /*
+     * qemu_fill_buffer might return just a few bytes, even when there isn't
+     * an error, so loop collecting them until we get enough.
+     */
+    while (pending < size) {
+        int received = qemu_fill_buffer(f);
+
+        if (received <= 0) {
+            break;
+        }
+
        index = f->buf_index + offset;
        pending = f->buf_size - index;
    }

@@ -709,6 +741,14 @@ int qemu_peek_buffer(QEMUFile *f, uint8_t *buf, int size, size_t offset)
    return size;
 }
 
+/*
+ * Read 'size' bytes of data from the file into buf.
+ * 'size' can be larger than the internal buffer.
+ *
+ * It will return size bytes unless there was an error, in which case it will
+ * return as many as it managed to read (assuming blocking fd's which
+ * all current QEMUFile are)
+ */
 int qemu_get_buffer(QEMUFile *f, uint8_t *buf, int size)
 {
    int pending = size;

@@ -717,7 +757,7 @@ int qemu_get_buffer(QEMUFile *f, uint8_t *buf, int size)
    while (pending > 0) {
        int res;
 
-        res = qemu_peek_buffer(f, buf, pending, 0);
+        res = qemu_peek_buffer(f, buf, MIN(pending, IO_BUF_SIZE), 0);
        if (res == 0) {
            return done;
        }

@@ -729,11 +769,16 @@ int qemu_get_buffer(QEMUFile *f, uint8_t *buf, int size)
    return done;
 }
 
+/*
+ * Peeks a single byte from the buffer; this isn't guaranteed to work if
+ * offset leaves a gap after the previous read/peeked data.
+ */
 int qemu_peek_byte(QEMUFile *f, int offset)
 {
    int index = f->buf_index + offset;
 
    assert(!qemu_file_is_writable(f));
+    assert(offset < IO_BUF_SIZE);
 

Quoted 6 lines

qemu-file.c +0 -0

    if (index >= f->buf_size) {
        qemu_fill_buffer(f);

Signature · 1 line

1.9.0

Quoted 8 lines

?? 2014??4??8????????10:29??Dr. David Alan Gilbert (git) <[email protected]> ??????

Quoted 5 lines

From: "Dr. David Alan Gilbert" <[email protected]>

Make qemu_peek_buffer repeatedly call fill_buffer until it gets all the data it requires, or until there is an error.

Quoted 8 lines

Quoted 6 lines

At the moment, qemu_peek_buffer will try one qemu_fill_buffer if there isn't enough data waiting, however the kernel is entitled to return just a few bytes, and still leave qemu_peek_buffer with less bytes than it needed. I've seen this fail in a dev world, and I think it could theoretically fail in the peeking of the subsection headers in the current world.

hmm, I also have got some errors(infrequently). Maybe It is one point. Could you show some messages about the error?

I've only seen the errors in my visitor/ber world where I use the peek_buffer a lot more; but the one place it is used in the existing code is in the code to check if we have the start of a subsection; if that goes wrong I'm not sure what error would be produced.

Dave

Quoted 152 lines

However, this patch avoids one potential issue, thanks.

Reviewed-by ChenLiang <[email protected]>

Quoted 147 lines

Comment qemu_peek_byte to point out it's not guaranteed to work for non-continuous peeks

Signed-off-by Dr. David Alan Gilbert <[email protected]>

---

v4:
Limit the size qemu_get_buffer passes to qemu_fill_buffer on each loop

v3:
Make qemu_fill_buffer return ssize_t
Remove the other size_t cleanup from this patch - I'll get back to them later Comment additions/cleanups from review
Stretch my -ve

include/migration/qemu-file.h | 5 ++++
qemu-file.c | 53 +++++++++++++++++++++++++++++++++++++++----

2 files changed, 54 insertions(+), 4 deletions(-)

include/migration/qemu-file.h +5 -0

index a191fb6..c90f529 100644
@@ -123,6 +123,11 @@ void qemu_put_be32(QEMUFile *f, unsigned int v);
 void qemu_put_be64(QEMUFile *f, uint64_t v);
 int qemu_peek_buffer(QEMUFile *f, uint8_t *buf, int size, size_t offset);
 int qemu_get_buffer(QEMUFile *f, uint8_t *buf, int size);
+/*
+ * Note that you can only peek continuous bytes from where the current pointer
+ * is; you aren't guaranteed to be able to peak to +n bytes unless you've
+ * previously peeked +n-1.
+ */
 int qemu_peek_byte(QEMUFile *f, int offset);
 int qemu_get_byte(QEMUFile *f);
 void qemu_file_skip(QEMUFile *f, int size);

qemu-file.c +49 -4

index e5ec798..6ae37d0 100644
@@ -529,7 +529,15 @@ size_t ram_control_save_page(QEMUFile *f, ram_addr_t block_offset,
   return RAM_SAVE_CONTROL_NOT_SUPP;
 }
 
-static void qemu_fill_buffer(QEMUFile *f)
+/*
+ * Attempt to fill the buffer from the underlying file
+ * Returns the number of bytes read, or negative value for an error.
+ *
+ * Note that it can return a partially full buffer even in a not error/not EOF
+ * case if the underlying file descriptor gives a short read, and that can
+ * happen even on a blocking fd.
+ */
+static ssize_t qemu_fill_buffer(QEMUFile *f)
 {
   int len;
   int pending;

@@ -553,6 +561,8 @@ static void qemu_fill_buffer(QEMUFile *f)
   } else if (len != -EAGAIN) {
       qemu_file_set_error(f, len);
   }
+
+    return len;
 }
 
 int qemu_get_fd(QEMUFile *f)

@@ -683,17 +693,39 @@ void qemu_file_skip(QEMUFile *f, int size)
   }
 }
 
+/*
+ * Read 'size' bytes from file (at 'offset') into buf without moving the
+ * pointer.
+ *
+ * It will return size bytes unless there was an error, in which case it will
+ * return as many as it managed to read (assuming blocking fd's which
+ * all current QEMUFile are)
+ */
 int qemu_peek_buffer(QEMUFile *f, uint8_t *buf, int size, size_t offset)
 {
   int pending;
   int index;
 
   assert(!qemu_file_is_writable(f));
+    assert(offset < IO_BUF_SIZE);
+    assert(size <= IO_BUF_SIZE - offset);
 
+    /* The 1st byte to read from */
   index = f->buf_index + offset;
+    /* The number of available bytes starting at index */
   pending = f->buf_size - index;
-    if (pending < size) {
-        qemu_fill_buffer(f);
+
+    /*
+     * qemu_fill_buffer might return just a few bytes, even when there isn't
+     * an error, so loop collecting them until we get enough.
+     */
+    while (pending < size) {
+        int received = qemu_fill_buffer(f);
+
+        if (received <= 0) {
+            break;
+        }
+
       index = f->buf_index + offset;
       pending = f->buf_size - index;
   }

@@ -709,6 +741,14 @@ int qemu_peek_buffer(QEMUFile *f, uint8_t *buf, int size, size_t offset)
   return size;
 }
 
+/*
+ * Read 'size' bytes of data from the file into buf.
+ * 'size' can be larger than the internal buffer.
+ *
+ * It will return size bytes unless there was an error, in which case it will
+ * return as many as it managed to read (assuming blocking fd's which
+ * all current QEMUFile are)
+ */
 int qemu_get_buffer(QEMUFile *f, uint8_t *buf, int size)
 {
   int pending = size;

@@ -717,7 +757,7 @@ int qemu_get_buffer(QEMUFile *f, uint8_t *buf, int size)
   while (pending > 0) {
       int res;
 
-        res = qemu_peek_buffer(f, buf, pending, 0);
+        res = qemu_peek_buffer(f, buf, MIN(pending, IO_BUF_SIZE), 0);
       if (res == 0) {
           return done;
       }

@@ -729,11 +769,16 @@ int qemu_get_buffer(QEMUFile *f, uint8_t *buf, int size)
   return done;
 }
 
+/*
+ * Peeks a single byte from the buffer; this isn't guaranteed to work if
+ * offset leaves a gap after the previous read/peeked data.
+ */
 int qemu_peek_byte(QEMUFile *f, int offset)
 {
   int index = f->buf_index + offset;
 
   assert(!qemu_file_is_writable(f));
+    assert(offset < IO_BUF_SIZE);
 

Quoted 7 lines

Quoted 6 lines

qemu-file.c +0 -0

   if (index >= f->buf_size) {
       qemu_fill_buffer(f);

Signature · 1 line

1.9.0

--
Dr. David Alan Gilbert / [email protected] / Manchester, UK

Quoted 19 lines · * ???? ([email protected]) wrote:

Quoted 16 lines

?? 2014??4??8????????10:29??Dr. David Alan Gilbert (git) <[email protected]> ??????

Quoted 11 lines

From: "Dr. David Alan Gilbert" <[email protected]>

Make qemu_peek_buffer repeatedly call fill_buffer until it gets all the data it requires, or until there is an error.

At the moment, qemu_peek_buffer will try one qemu_fill_buffer if there isn't enough data waiting, however the kernel is entitled to return just a few bytes, and still leave qemu_peek_buffer with less bytes than it needed. I've seen this fail in a dev world, and I think it could theoretically fail in the peeking of the subsection headers in the current world.

hmm, I also have got some errors(infrequently). Maybe It is one point. Could you show some messages about the error?

Quoted 6 lines

I've only seen the errors in my visitor/ber world where I use the peek_buffer a lot more; but the one place it is used in the existing code is in the code to check if we have the start of a subsection; if that goes wrong I'm not sure what error would be produced.

Dave

In my observation, error is more likely to happen if migration thread is blocked too long. BTW, xbzrle and auto convergence are disable.

Quoted 24 lines

Quoted 24 lines · * ???? ([email protected]) wrote:

Quoted 8 lines

?? 2014??4??8????????10:29??Dr. David Alan Gilbert (git) <[email protected]> ??????

Quoted 5 lines

From: "Dr. David Alan Gilbert" <[email protected]>

Make qemu_peek_buffer repeatedly call fill_buffer until it gets all the data it requires, or until there is an error.

Quoted 8 lines

Quoted 6 lines

At the moment, qemu_peek_buffer will try one qemu_fill_buffer if there isn't enough data waiting, however the kernel is entitled to return just a few bytes, and still leave qemu_peek_buffer with less bytes than it needed. I've seen this fail in a dev world, and I think it could theoretically fail in the peeking of the subsection headers in the current world.

hmm, I also have got some errors(infrequently). Maybe It is one point. Could you show some messages about the error?

I've only seen the errors in my visitor/ber world where I use the peek_buffer a lot more; but the one place it is used in the existing code is in the code to check if we have the start of a subsection; if that goes wrong I'm not sure what error would be produced.

Quoted 3 lines

Quoted 1 line

Dave

In my observation, error is more likely to happen if migration thread is blocked too long. BTW, xbzrle and auto convergence are disable.

What error do you see?

Dave

--
Dr. David Alan Gilbert / [email protected] / Manchester, UK

Quoted 26 lines · * ???? ([email protected]) wrote:

Quoted 24 lines

Quoted 24 lines · * ???? ([email protected]) wrote:

Quoted 8 lines

?? 2014??4??8????????10:29??Dr. David Alan Gilbert (git) <[email protected]> ??????

Quoted 5 lines

From: "Dr. David Alan Gilbert" <[email protected]>

Make qemu_peek_buffer repeatedly call fill_buffer until it gets all the data it requires, or until there is an error.

Quoted 8 lines

Quoted 6 lines

At the moment, qemu_peek_buffer will try one qemu_fill_buffer if there isn't enough data waiting, however the kernel is entitled to return just a few bytes, and still leave qemu_peek_buffer with less bytes than it needed. I've seen this fail in a dev world, and I think it could theoretically fail in the peeking of the subsection headers in the current world.

hmm, I also have got some errors(infrequently). Maybe It is one point. Could you show some messages about the error?

I've only seen the errors in my visitor/ber world where I use the peek_buffer a lot more; but the one place it is used in the existing code is in the code to check if we have the start of a subsection; if that goes wrong I'm not sure what error would be produced.

Quoted 7 lines

Quoted 3 lines

Quoted 1 line

Dave

In my observation, error is more likely to happen if migration thread is blocked too long. BTW, xbzrle and auto convergence are disable.

What error do you see?

Dave

It likes section id 1 load failed at dest side.