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.

Ditto for qemu_peek_byte (which can only be affected due to it's offset).

Simplify qemu_get_buffer since it can now rely on qemu_peek_buffer to loop.

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

qemu-file.c +3 -8

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

Ditto for qemu_peek_byte (which can only be affected due to it's offset).

Simplify qemu_get_buffer since it can now rely on qemu_peek_buffer to loop.

I think this one is wrong, will explain there.

Quoted 2 lines

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

Nice.

Quoted 20 lines

qemu-file.c +3 -14

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

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

Quoted 4 lines

qemu-file.c +3 -1

-    return done;
+    qemu_file_skip(f, res);
+
+    return res;

I think this is "theoretical" (a.k.a. no user of this functionality). As this was coded, we could receive buffers bigger than IOBUF_SIZE, with your change, we can't. Just maintating the loop should fix this, right?

qemu-file.c int qemu_peek_byte(QEMUFile *f, int offset) · line 755

Quoted 4 lines

qemu-file.c +4 -0

+    while (index >= f->buf_size) {
+        int received = qemu_fill_buffer(f);
+
+        if (received <= 0) {

here, I don't know really what to do. We just need one character, so the 1st call to qemu_fill_buffer() gives it to us, or we are already on problems. i.e. no need of the while() loop.

On the other hand, having exactly the same code looks so nice.

At some point I was thinking about making qemu_peek_byte() to use qemu_peek_buffer(), but I think that we used qemu_peek_byte() more to justify the overhead. I am talking from memory here.

vmstate.c static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd, · line 175

Quoted 12 lines

vmstate.c +1 -1

index d1f5eb0..b8e6e31 100644
@@ -170,7 +170,7 @@ static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
         }
         size = qemu_peek_buffer(f, (uint8_t *)idstr, len, 2);
         if (size != len) {
-            return 0;
+            return -EIO;
         }
         idstr[size] = 0;

This was coded this way on purpose. If we don't have a valid buffer after the subsection identifier, just let the code continue to see if it wasn't a subsection at all. This colud be removed one tested that we don't allow subsections in the middle of a section, only in places where a section can appear.

In general, very nice patch, and fixes the problem.

Later, Juan.

Quoted 5 lines

Quoted 4 lines

qemu-file.c +4 -0

+    while (index >= f->buf_size) {
+        int received = qemu_fill_buffer(f);
+
+        if (received <= 0) {

Quoted 3 lines

here, I don't know really what to do. We just need one character, so the 1st call to qemu_fill_buffer() gives it to us, or we are already on problems. i.e. no need of the while() loop.

The problem is that peek_byte takes an offset, so while qemu_fill_buffer will get us a byte, we actually need it to get us all the bytes upto the offset, and that's not guaranteed from one call.

Quoted 5 lines · Dr. David Alan Gilbert

Quoted 5 lines

Quoted 4 lines

qemu-file.c +4 -0

+    while (index >= f->buf_size) {
+        int received = qemu_fill_buffer(f);
+
+        if (received <= 0) {

Quoted 7 lines

Quoted 3 lines

here, I don't know really what to do. We just need one character, so the 1st call to qemu_fill_buffer() gives it to us, or we are already on problems. i.e. no need of the while() loop.

The problem is that peek_byte takes an offset, so while qemu_fill_buffer will get us a byte, we actually need it to get us all the bytes upto the offset, and that's not guaranteed from one call.

that is not a problem.

We never got a "hole" on the things that we ask for. We ask for the "next byte", or the "next bytes", so in qemu_peek_byte() we are guaranteed (with current users) that we would only have to read a single byte.

I.e. qemu_peek_{buffer,byte}(....,offset, size) means that we have "already" peek until offset, and now we want size more bytes.

Quoted 21 lines · * Juan Quintela ([email protected]) wrote:

Quoted 5 lines · Dr. David Alan Gilbert

Quoted 5 lines

Quoted 4 lines

qemu-file.c +4 -0

+    while (index >= f->buf_size) {
+        int received = qemu_fill_buffer(f);
+
+        if (received <= 0) {

Quoted 7 lines

Quoted 3 lines

here, I don't know really what to do. We just need one character, so the 1st call to qemu_fill_buffer() gives it to us, or we are already on problems. i.e. no need of the while() loop.

The problem is that peek_byte takes an offset, so while qemu_fill_buffer will get us a byte, we actually need it to get us all the bytes upto the offset, and that's not guaranteed from one call.

that is not a problem.

We never got a "hole" on the things that we ask for. We ask for the "next byte", or the "next bytes", so in qemu_peek_byte() we are guaranteed (with current users) that we would only have to read a single byte.

Quoted 2 lines

I.e. qemu_peek_{buffer,byte}(....,offset, size) means that we have "already" peek until offset, and now we want size more bytes.

Hmm ok, that is convenient but is neither documented or enforced; I'll rework my patch and at least document it, and possibly enforce it.