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

[Uri Lublin <uri@il.qumranet.com>]

Anthony Liguori wrote:
> 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.
> 

Hi Anthony,

Thanks for pushing Live Migration into upstream qemu.

This patch doesn't provide us with full duplex capability yet:
  * qemu_fopen_fd() now only register get_buffer function (always a reader).
  * Also there still a single buffer that should not be shared by reader/writer.
  * It aborts upon a read-to-write switch and upon a write-to-read switch. That
    as you've mentioned may be helpful when/if implementing a full duplex
    qemu_file_* mechanism.

Below are some specific code comments.

Thanks,
Uri.

 >@@ -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;

Better return the size that was actually written

 > }
 >
 > 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;

Same here

 > }



 > 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;

Untabify.

 >         f->buf_index = 0;
 >     }
 > }