[Qemu-devel] [PATCH v4 1/1] Make qemu_peek_buffer loop until it gets it's data

["Dr. David Alan Gilbert (git)" <dgilbert@redhat.com>]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

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.

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

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---

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

diff --git a/include/migration/qemu-file.h b/include/migration/qemu-file.h
index a191fb6..c90f529 100644
--- a/include/migration/qemu-file.h
+++ b/include/migration/qemu-file.h
@@ -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);
diff --git a/qemu-file.c b/qemu-file.c
index e5ec798..6ae37d0 100644
--- a/qemu-file.c
+++ b/qemu-file.c
@@ -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);
 
     if (index >= f->buf_size) {
         qemu_fill_buffer(f);
-- 
1.9.0