Make qemu_peek_buffer loop until it gets it's data

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

Make qemu_peek_buffer repatedly 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

+/*
+ * Attempt to fill the buffer from the underlying file
+ * Returns the number of bytes read, or -ve value for an error.

Quoted 12 lines · Dr. David Alan Gilbert (git)

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

Make qemu_peek_buffer repatedly 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.

Quoted 5 lines

Comment qemu_peek_byte to point out it's not guaranteed to work for

+/*
+ * Attempt to fill the buffer from the underlying file
+ * Returns the number of bytes read, or -ve value for an error.

s/-ve/negative/ - it is not an obvious abbreviation, and I only knew what it meant because you have been told to fix it in other patches.

Quoted 2 lines

s/-ve/negative/ - it is not an obvious abbreviation, and I only knew what it meant because you have been told to fix it in other patches.

If I need to recut it for another reason I'll change it. However, it's a perfectly common abbreviation that's widely used (hundreds of times in the kernel source for example).

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

Make qemu_peek_buffer repatedly 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

Use size_t rather than int for size parameters, (and result for those functions that never return -errno).

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

---
 include/migration/qemu-file.h | 13 +++++++----
 qemu-file.c                   | 53 +++++++++++++++++++++++++++++++++++--------
 2 files changed, 52 insertions(+), 14 deletions(-)

include/migration/qemu-file.h +9 -4

index a191fb6..6dd728d 100644
@@ -121,11 +121,16 @@ static inline void qemu_put_ubyte(QEMUFile *f, unsigned int v)
 void qemu_put_be16(QEMUFile *f, unsigned int v);
 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);
-int qemu_peek_byte(QEMUFile *f, int offset);
+size_t qemu_peek_buffer(QEMUFile *f, uint8_t *buf, size_t size, size_t offset);
+size_t qemu_get_buffer(QEMUFile *f, uint8_t *buf, size_t 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, size_t offset);
 int qemu_get_byte(QEMUFile *f);
-void qemu_file_skip(QEMUFile *f, int size);
+void qemu_file_skip(QEMUFile *f, size_t size);
 void qemu_update_position(QEMUFile *f, size_t size);
 
 static inline unsigned int qemu_get_ubyte(QEMUFile *f)

qemu-file.c +0 -0

index e5ec798..d426136 100644

qemu-file.c +3 -1

 {
     int len;
     int pending;

@@ -553,6 +557,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)

@@ -676,24 +682,40 @@ void qemu_put_byte(QEMUFile *f, int v)
     }
 }
 
-void qemu_file_skip(QEMUFile *f, int size)
+void qemu_file_skip(QEMUFile *f, size_t size)
 {
     if (f->buf_index + size <= f->buf_size) {
         f->buf_index += size;

qemu-file.c +2 -2

+    /* 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-file.c +5 -3

+ */
+size_t qemu_get_buffer(QEMUFile *f, uint8_t *buf, size_t size)
 {
-    int pending = size;
-    int done = 0;
+    size_t pending = size;
+    size_t done = 0;
 
     while (pending > 0) {
-        int res;
+        size_t res;
 
         res = qemu_peek_buffer(f, buf, pending, 0);
         if (res == 0) {

@@ -729,7 +758,11 @@ int qemu_get_buffer(QEMUFile *f, uint8_t *buf, int size)
     return done;

Quoted 12 lines · Dr. David Alan Gilbert (git)

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

Make qemu_peek_buffer repatedly 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.

Quoted 5 lines

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

Use size_t rather than int for size parameters, (and result for those functions that never return -errno).

Have you considered doing this cleanup in a separate patch?

Are there any "size or -errno" function values? If yes, recommend to make them ssize_t.

Quoted 40 lines

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

---
 include/migration/qemu-file.h | 13 +++++++----
 qemu-file.c                   | 53 +++++++++++++++++++++++++++++++++++--------
 2 files changed, 52 insertions(+), 14 deletions(-)

include/migration/qemu-file.h +9 -4

index a191fb6..6dd728d 100644
@@ -121,11 +121,16 @@ static inline void qemu_put_ubyte(QEMUFile *f, unsigned int v)
 void qemu_put_be16(QEMUFile *f, unsigned int v);
 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);
-int qemu_peek_byte(QEMUFile *f, int offset);
+size_t qemu_peek_buffer(QEMUFile *f, uint8_t *buf, size_t size, size_t offset);
+size_t qemu_get_buffer(QEMUFile *f, uint8_t *buf, size_t 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, size_t offset);
 int qemu_get_byte(QEMUFile *f);
-void qemu_file_skip(QEMUFile *f, int size);
+void qemu_file_skip(QEMUFile *f, size_t size);
 void qemu_update_position(QEMUFile *f, size_t size);
 
 static inline unsigned int qemu_get_ubyte(QEMUFile *f)

qemu-file.c +0 -0

index e5ec798..d426136 100644
@@ -529,7 +529,11 @@ size_t ram_control_save_page(QEMUFile *f, ram_addr_t block_offset,
     return RAM_SAVE_CONTROL_NOT_SUPP;
 }
 

qemu-file.c size_t ram_control_save_page(QEMUFile *f, ram_addr_t block_offset, · line 534

Quoted 4 lines

qemu-file.c +3 -1

-static void qemu_fill_buffer(QEMUFile *f)
+/*
+ * Attempt to fill the buffer from the underlying file
+ * Returns the number of bytes read, or -ve value for an error.

Please spell out negative. The clarity gained is well worth five characters.

Suggest to spell out that this can succeed without filling the buffer completely, and not just because when hitting EOF.

qemu-file.c size_t ram_control_save_page(QEMUFile *f, ram_addr_t block_offset, · line 536

Quoted 2 lines

qemu-file.c +2 -0

+ */
+static int qemu_fill_buffer(QEMUFile *f)

Aha, here's a function value that could become ssize_t. But then QEMUFileGetBufferFunc & friends should also be changed. Feels even more like a separate patch.

Quoted 24 lines

qemu-file.c +3 -1

 {
     int len;
     int pending;

@@ -553,6 +557,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)

@@ -676,24 +682,40 @@ void qemu_put_byte(QEMUFile *f, int v)
     }
 }
 
-void qemu_file_skip(QEMUFile *f, int size)
+void qemu_file_skip(QEMUFile *f, size_t size)
 {
     if (f->buf_index + size <= f->buf_size) {
         f->buf_index += size;
     }
 }
 

qemu-file.c void qemu_put_byte(QEMUFile *f, int v) · line 697

Quoted 7 lines

qemu-file.c +6 -1

-int qemu_peek_buffer(QEMUFile *f, uint8_t *buf, int size, size_t offset)
+/*
+ * Read 'size' bytes from file (at 'offset') into buf without moving the
+ * pointer.
+ *
+ * If the underlying fd blocks, then it will return size bytes unless there
+ * was an error, in which case it will return as many as it managed to read.

Begs the question what it'll do when the fd doesn't block.

qemu-file.c void qemu_put_byte(QEMUFile *f, int v) · line 706

Quoted 9 lines

qemu-file.c +4 -0

+ */
+size_t qemu_peek_buffer(QEMUFile *f, uint8_t *buf, size_t size, size_t offset)
 {
     int pending;
     int index;
 
     assert(!qemu_file_is_writable(f));
+    assert(offset < IO_BUF_SIZE);
+    assert(size + offset < IO_BUF_SIZE);

Off-by-one? offset + size <= IO_BUF_SIZE

If you want to guard against overflow: size <= IO_BUF_SIZE - offset.

Quoted 12 lines

qemu-file.c +7 -2

+    /* 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);
+    while (pending < size) {
+        int received = qemu_fill_buffer(f);
+
+        if (received <= 0) {
+            break;

qemu-file.c void qemu_put_byte(QEMUFile *f, int v) · line 721

Quoted 5 lines

qemu-file.c +2 -0

+        }
+
         index = f->buf_index + offset;
         pending = f->buf_size - index;
     }

Loop is useful since qemu_fill_buffer() can succeed without filling the buffer, and trying again can get it filled. Correct?

Quoted 4 lines

qemu-file.c +0 -0

@@ -709,13 +731,20 @@ int qemu_peek_buffer(QEMUFile *f, uint8_t *buf, int size, size_t offset)
     return size;
 }
 

qemu-file.c int qemu_peek_buffer(QEMUFile *f, uint8_t *buf, int size, size_t offset) · line 739

Quoted 7 lines

qemu-file.c +6 -1

-int qemu_get_buffer(QEMUFile *f, uint8_t *buf, int size)
+/*
+ * Read 'size' bytes of data from the file into buf.
+ * 'size' can be larger than the internal buffer.
+ *
+ * If the underlying fd blocks, then it will return size bytes unless there
+ * was an error, in which case it will return as many as it managed to read.

Begs the question what it'll do when the fd doesn't block.

Quoted 17 lines

qemu-file.c +4 -3

+size_t qemu_get_buffer(QEMUFile *f, uint8_t *buf, size_t size)
 {
-    int pending = size;
-    int done = 0;
+    size_t pending = size;
+    size_t done = 0;
 
     while (pending > 0) {
-        int res;
+        size_t res;
 
         res = qemu_peek_buffer(f, buf, pending, 0);
         if (res == 0) {

@@ -729,7 +758,11 @@ int qemu_get_buffer(QEMUFile *f, uint8_t *buf, int size)
     return done;
 }
 

qemu-file.c int qemu_get_buffer(QEMUFile *f, uint8_t *buf, int size) · line 767

Quoted 8 lines

qemu-file.c +5 -1

-int qemu_peek_byte(QEMUFile *f, int offset)
+/*
+ * 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, size_t offset)
 {
     int index = f->buf_index + offset;

assert the offset is sane, like you did in qemu_peek_buffer()?

Quoted 14 lines · * Markus Armbruster ([email protected]) wrote:

Quoted 12 lines · Dr. David Alan Gilbert (git)

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

Make qemu_peek_buffer repatedly 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.

Quoted 7 lines

Quoted 5 lines

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

Use size_t rather than int for size parameters, (and result for those functions that never return -errno).

Have you considered doing this cleanup in a separate patch?

I'd just taken the ones involved in the functions I was changing anyway, and it seemed small enough to roll in, but yes I can do that.

Quoted 2 lines

Are there any "size or -errno" function values? If yes, recommend to make them ssize_t.

Yes there are a few.

Quoted 40 lines

Quoted 40 lines

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

---
 include/migration/qemu-file.h | 13 +++++++----
 qemu-file.c                   | 53 +++++++++++++++++++++++++++++++++++--------
 2 files changed, 52 insertions(+), 14 deletions(-)

include/migration/qemu-file.h +9 -4

index a191fb6..6dd728d 100644
@@ -121,11 +121,16 @@ static inline void qemu_put_ubyte(QEMUFile *f, unsigned int v)
 void qemu_put_be16(QEMUFile *f, unsigned int v);
 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);
-int qemu_peek_byte(QEMUFile *f, int offset);
+size_t qemu_peek_buffer(QEMUFile *f, uint8_t *buf, size_t size, size_t offset);
+size_t qemu_get_buffer(QEMUFile *f, uint8_t *buf, size_t 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, size_t offset);
 int qemu_get_byte(QEMUFile *f);
-void qemu_file_skip(QEMUFile *f, int size);
+void qemu_file_skip(QEMUFile *f, size_t size);
 void qemu_update_position(QEMUFile *f, size_t size);
 
 static inline unsigned int qemu_get_ubyte(QEMUFile *f)

qemu-file.c +0 -0

index e5ec798..d426136 100644
@@ -529,7 +529,11 @@ size_t ram_control_save_page(QEMUFile *f, ram_addr_t block_offset,
     return RAM_SAVE_CONTROL_NOT_SUPP;
 }
 

Quoted 7 lines

qemu-file.c size_t ram_control_save_page(QEMUFile *f, ram_addr_t block_offset, · line 534

Quoted 4 lines

qemu-file.c +3 -1

-static void qemu_fill_buffer(QEMUFile *f)
+/*
+ * Attempt to fill the buffer from the underlying file
+ * Returns the number of bytes read, or -ve value for an error.

Please spell out negative. The clarity gained is well worth five characters.

I can see I'm going to need a mod to checkpatch for this....

Quoted 2 lines

Suggest to spell out that this can succeed without filling the buffer completely, and not just because when hitting EOF.

Yep I can clarify that.

Quoted 6 lines

qemu-file.c size_t ram_control_save_page(QEMUFile *f, ram_addr_t block_offset, · line 536

Quoted 2 lines

qemu-file.c +2 -0

+ */
+static int qemu_fill_buffer(QEMUFile *f)

Aha, here's a function value that could become ssize_t. But then QEMUFileGetBufferFunc & friends should also be changed. Feels even more like a separate patch.

Yes it could; I'd not considered that but it does make more sense than
int. However, changing that will mean I also need to change more other
places, so yeh, separate patch.

Quoted 24 lines

Quoted 24 lines

qemu-file.c +3 -1

 {
     int len;
     int pending;

@@ -553,6 +557,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)

@@ -676,24 +682,40 @@ void qemu_put_byte(QEMUFile *f, int v)
     }
 }
 
-void qemu_file_skip(QEMUFile *f, int size)
+void qemu_file_skip(QEMUFile *f, size_t size)
 {
     if (f->buf_index + size <= f->buf_size) {
         f->buf_index += size;
     }
 }
 

Quoted 9 lines

qemu-file.c void qemu_put_byte(QEMUFile *f, int v) · line 697

Quoted 7 lines

qemu-file.c +6 -1

-int qemu_peek_buffer(QEMUFile *f, uint8_t *buf, int size, size_t offset)
+/*
+ * Read 'size' bytes from file (at 'offset') into buf without moving the
+ * pointer.
+ *
+ * If the underlying fd blocks, then it will return size bytes unless there
+ * was an error, in which case it will return as many as it managed to read.

Begs the question what it'll do when the fd doesn't block.

At the moment the migration stream stuff is always set up to block (although in the postcopy world that's changed and this becomes more 'interesting'), still if it begs that question then at the moment it's undefined, and I don't see a reason to define it until we use it that way/figure out what the best thing is.

Quoted 11 lines

qemu-file.c void qemu_put_byte(QEMUFile *f, int v) · line 706

Quoted 9 lines

qemu-file.c +4 -0

+ */
+size_t qemu_peek_buffer(QEMUFile *f, uint8_t *buf, size_t size, size_t offset)
 {
     int pending;
     int index;
 
     assert(!qemu_file_is_writable(f));
+    assert(offset < IO_BUF_SIZE);
+    assert(size + offset < IO_BUF_SIZE);

Off-by-one? offset + size <= IO_BUF_SIZE

Hmm good catch.

Quoted 14 lines

If you want to guard against overflow: size <= IO_BUF_SIZE - offset.

Quoted 12 lines

qemu-file.c +7 -2

+    /* 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);
+    while (pending < size) {
+        int received = qemu_fill_buffer(f);
+
+        if (received <= 0) {
+            break;

Quoted 8 lines

qemu-file.c void qemu_put_byte(QEMUFile *f, int v) · line 721

Quoted 5 lines

qemu-file.c +2 -0

+        }
+
         index = f->buf_index + offset;
         pending = f->buf_size - index;
     }

Loop is useful since qemu_fill_buffer() can succeed without filling the buffer, and trying again can get it filled. Correct?

Correct; I'll add a comment to make it explicit.

Quoted 4 lines

Quoted 4 lines

qemu-file.c +0 -0

@@ -709,13 +731,20 @@ int qemu_peek_buffer(QEMUFile *f, uint8_t *buf, int size, size_t offset)
     return size;
 }
 

Quoted 9 lines

qemu-file.c int qemu_peek_buffer(QEMUFile *f, uint8_t *buf, int size, size_t offset) · line 739

Quoted 7 lines

qemu-file.c +6 -1

-int qemu_get_buffer(QEMUFile *f, uint8_t *buf, int size)
+/*
+ * Read 'size' bytes of data from the file into buf.
+ * 'size' can be larger than the internal buffer.
+ *
+ * If the underlying fd blocks, then it will return size bytes unless there
+ * was an error, in which case it will return as many as it managed to read.

Begs the question what it'll do when the fd doesn't block.

As above.

Quoted 17 lines

Quoted 17 lines

qemu-file.c +4 -3

+size_t qemu_get_buffer(QEMUFile *f, uint8_t *buf, size_t size)
 {
-    int pending = size;
-    int done = 0;
+    size_t pending = size;
+    size_t done = 0;
 
     while (pending > 0) {
-        int res;
+        size_t res;
 
         res = qemu_peek_buffer(f, buf, pending, 0);
         if (res == 0) {

@@ -729,7 +758,11 @@ int qemu_get_buffer(QEMUFile *f, uint8_t *buf, int size)
     return done;
 }
 

Quoted 10 lines

qemu-file.c int qemu_get_buffer(QEMUFile *f, uint8_t *buf, int size) · line 767

Quoted 8 lines

qemu-file.c +5 -1

-int qemu_peek_byte(QEMUFile *f, int offset)
+/*
+ * 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, size_t offset)
 {
     int index = f->buf_index + offset;

assert the offset is sane, like you did in qemu_peek_buffer()?

Yep, can do.