Re: [PATCH] qemu: qemu_fopen_fd: differentiate between reader and writer user

[Anthony Liguori <anthony@codemonkey.ws>]

[-- Attachment #1: Type: text/plain, Size: 368 bytes --]

Anthony Liguori wrote:
>
> Also, having checks and the read and write functions to determine if 
> the is_write flag is set along with whether buf_index > 0 that 
> fprintf()'d and aborted would be good for debugging.

I have a patch that does this along with fixing a few other bugs.  It's 
attached.

Regards,

Anthony Liguori

>
> Regards,
>
> Anthony Liguori
>
>


[-- Attachment #2: 006_live_migration.patch --]
[-- Type: text/x-patch, Size: 8114 bytes --]

diff --git a/hw/hw.h b/hw/hw.h
index e130355..8edd788 100644
--- a/hw/hw.h
+++ b/hw/hw.h
@@ -11,8 +11,8 @@
  * The pos argument can be ignored if the file is only being used for
  * streaming.  The handler should try to write all of the data it can.
  */
-typedef void (QEMUFilePutBufferFunc)(void *opaque, const uint8_t *buf,
-                                     int64_t pos, int size);
+typedef int (QEMUFilePutBufferFunc)(void *opaque, const uint8_t *buf,
+                                    int64_t pos, int size);
 
 /* Read a chunk of data from a file at the given position.  The pos argument
  * can be ignored if the file is only be used for streaming.  The number of
@@ -64,6 +64,7 @@ unsigned int qemu_get_be16(QEMUFile *f);
 unsigned int qemu_get_be32(QEMUFile *f);
 uint64_t qemu_get_be64(QEMUFile *f);
 int qemu_file_rate_limit(QEMUFile *f);
+int qemu_file_has_error(QEMUFile *f);
 
 /* Try to send any outstanding data.  This function is useful when output is
  * halted due to rate limiting or EAGAIN errors occur as it can be used to
diff --git a/vl.c b/vl.c
index 5659fea..d49c648 100644
--- a/vl.c
+++ b/vl.c
@@ -6197,12 +6197,15 @@ struct QEMUFile {
     QEMUFileCloseFunc *close;
     QEMUFileRateLimit *rate_limit;
     void *opaque;
+    int is_write;
 
     int64_t buf_offset; /* start of buffer when writing, end of buffer
                            when reading */
     int buf_index;
     int buf_size; /* 0 when writing */
     uint8_t buf[IO_BUF_SIZE];
+
+    int has_error;
 };
 
 typedef struct QEMUFileFD
@@ -6211,34 +6214,6 @@ typedef struct QEMUFileFD
     QEMUFile *file;
 } QEMUFileFD;
 
-static void fd_put_notify(void *opaque)
-{
-    QEMUFileFD *s = opaque;
-
-    /* Remove writable callback and do a put notify */
-    qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
-    qemu_file_put_notify(s->file);
-}
-
-static void fd_put_buffer(void *opaque, const uint8_t *buf,
-                          int64_t pos, int size)
-{
-    QEMUFileFD *s = opaque;
-    ssize_t len;
-
-    do {
-        len = write(s->fd, buf, size);
-    } while (len == -1 && errno == EINTR);
-
-    if (len == -1)
-        len = -errno;
-
-    /* When the fd becomes writable again, register a callback to do
-     * a put notify */
-    if (len == -EAGAIN)
-        qemu_set_fd_handler2(s->fd, NULL, NULL, fd_put_notify, s);
-}
-
 static int fd_get_buffer(void *opaque, uint8_t *buf, int64_t pos, int size)
 {
     QEMUFileFD *s = opaque;
@@ -6269,7 +6244,7 @@ QEMUFile *qemu_fopen_fd(int fd)
         return NULL;
 
     s->fd = fd;
-    s->file = qemu_fopen_ops(s, fd_put_buffer, fd_get_buffer, fd_close, NULL);
+    s->file = qemu_fopen_ops(s, NULL, fd_get_buffer, fd_close, NULL);
     return s->file;
 }
 
@@ -6278,12 +6253,13 @@ typedef struct QEMUFileStdio
     FILE *outfile;
 } QEMUFileStdio;
 
-static void file_put_buffer(void *opaque, const uint8_t *buf,
+static int file_put_buffer(void *opaque, const uint8_t *buf,
                             int64_t pos, int size)
 {
     QEMUFileStdio *s = opaque;
     fseek(s->outfile, pos, SEEK_SET);
     fwrite(buf, 1, size, s->outfile);
+    return size;
 }
 
 static int file_get_buffer(void *opaque, uint8_t *buf, int64_t pos, int size)
@@ -6331,11 +6307,12 @@ typedef struct QEMUFileBdrv
     int64_t base_offset;
 } QEMUFileBdrv;
 
-static void bdrv_put_buffer(void *opaque, const uint8_t *buf,
-                            int64_t pos, int size)
+static int bdrv_put_buffer(void *opaque, const uint8_t *buf,
+                           int64_t pos, int size)
 {
     QEMUFileBdrv *s = opaque;
     bdrv_pwrite(s->bs, s->base_offset + pos, buf, size);
+    return size;
 }
 
 static int bdrv_get_buffer(void *opaque, uint8_t *buf, int64_t pos, int size)
@@ -6384,18 +6361,29 @@ QEMUFile *qemu_fopen_ops(void *opaque, QEMUFilePutBufferFunc *put_buffer,
     f->get_buffer = get_buffer;
     f->close = close;
     f->rate_limit = rate_limit;
+    f->is_write = 0;
 
     return f;
 }
 
+int qemu_file_has_error(QEMUFile *f)
+{
+    return f->has_error;
+}
+
 void qemu_fflush(QEMUFile *f)
 {
     if (!f->put_buffer)
         return;
 
-    if (f->buf_index > 0) {
-        f->put_buffer(f->opaque, f->buf, f->buf_offset, f->buf_index);
-        f->buf_offset += f->buf_index;
+    if (f->is_write && f->buf_index > 0) {
+        int len;
+
+	len = f->put_buffer(f->opaque, f->buf, f->buf_offset, f->buf_index);
+	if (len > 0)
+	    f->buf_offset += f->buf_index;
+	else
+	    f->has_error = 1;
         f->buf_index = 0;
     }
 }
@@ -6407,13 +6395,16 @@ static void qemu_fill_buffer(QEMUFile *f)
     if (!f->get_buffer)
         return;
 
-    len = f->get_buffer(f->opaque, f->buf, f->buf_offset, IO_BUF_SIZE);
-    if (len < 0)
-        len = 0;
+    if (f->is_write)
+        abort();
 
-    f->buf_index = 0;
-    f->buf_size = len;
-    f->buf_offset += len;
+    len = f->get_buffer(f->opaque, f->buf, f->buf_offset, IO_BUF_SIZE);
+    if (len > 0) {
+	f->buf_index = 0;
+	f->buf_size = len;
+	f->buf_offset += len;
+    } else if (len != -EAGAIN)
+	f->has_error = 1;
 }
 
 int qemu_fclose(QEMUFile *f)
@@ -6434,11 +6425,19 @@ void qemu_file_put_notify(QEMUFile *f)
 void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, int size)
 {
     int l;
-    while (size > 0) {
+
+    if (!f->has_error && f->is_write == 0 && f->buf_index > 0) {
+        fprintf(stderr,
+                "Attempted to write to buffer while read buffer is not empty\n");
+        abort();
+    }
+
+    while (!f->has_error && size > 0) {
         l = IO_BUF_SIZE - f->buf_index;
         if (l > size)
             l = size;
         memcpy(f->buf + f->buf_index, buf, l);
+        f->is_write = 1;
         f->buf_index += l;
         buf += l;
         size -= l;
@@ -6449,7 +6448,14 @@ void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, int size)
 
 void qemu_put_byte(QEMUFile *f, int v)
 {
+    if (!f->has_error && f->is_write == 0 && f->buf_index > 0) {
+        fprintf(stderr,
+                "Attempted to write to buffer while read buffer is not empty\n");
+        abort();
+    }
+
     f->buf[f->buf_index++] = v;
+    f->is_write = 1;
     if (f->buf_index >= IO_BUF_SIZE)
         qemu_fflush(f);
 }
@@ -6458,6 +6464,9 @@ int qemu_get_buffer(QEMUFile *f, uint8_t *buf, int size1)
 {
     int size, l;
 
+    if (f->is_write)
+        abort();
+
     size = size1;
     while (size > 0) {
         l = f->buf_size - f->buf_index;
@@ -6479,6 +6488,9 @@ int qemu_get_buffer(QEMUFile *f, uint8_t *buf, int size1)
 
 int qemu_get_byte(QEMUFile *f)
 {
+    if (f->is_write)
+        abort();
+
     if (f->buf_index >= f->buf_size) {
         qemu_fill_buffer(f);
         if (f->buf_index >= f->buf_size)
@@ -6671,6 +6683,9 @@ int qemu_savevm_state_begin(QEMUFile *f)
         se->save_live_state(f, QEMU_VM_SECTION_START, se->opaque);
     }
 
+    if (qemu_file_has_error(f))
+        return -EIO;
+
     return 0;
 }
 
@@ -6693,6 +6708,9 @@ int qemu_savevm_state_iterate(QEMUFile *f)
     if (ret)
         return 1;
 
+    if (qemu_file_has_error(f))
+        return -EIO;
+
     return 0;
 }
 
@@ -6734,6 +6752,9 @@ int qemu_savevm_state_complete(QEMUFile *f)
 
     qemu_put_byte(f, QEMU_VM_EOF);
 
+    if (qemu_file_has_error(f))
+	return -EIO;
+
     return 0;
 }
 
@@ -6758,8 +6779,12 @@ int qemu_savevm_state(QEMUFile *f)
     ret = qemu_savevm_state_complete(f);
 
 out:
-    if (saved_vm_running)
+    if (qemu_file_has_error(f))
+	ret = -EIO;
+
+    if (!ret && saved_vm_running)
         vm_start();
+
     return ret;
 }
 
@@ -6815,6 +6840,10 @@ static int qemu_loadvm_state_v2(QEMUFile *f)
         /* always seek to exact end of record */
         qemu_fseek(f, cur_pos + record_len, SEEK_SET);
     }
+
+    if (qemu_file_has_error(f))
+	return -EIO;
+
     return 0;
 }
 
@@ -6913,6 +6942,9 @@ out:
         qemu_free(le);
     }
 
+    if (qemu_file_has_error(f))
+        ret = -EIO;
+
     return ret;
 }
 
@@ -7224,6 +7256,10 @@ static int ram_get_page(QEMUFile *f, uint8_t *buf, int len)
     default:
         return -EINVAL;
     }
+
+    if (qemu_file_has_error(f))
+	return -EIO;
+
     return 0;
 }