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

[Anthony Liguori <anthony@codemonkey.ws>]

Uri Lublin wrote:
> Anthony Liguori wrote:

I have already cut your text, but I don't understand the comment about 
not being "full duplex".  Is there a reason why migration needs to be 
bidirectional?  I don't think there's a fundamental reason it needs to 
be and I think there are some advantages to it being unidirectional.

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

At this level in the API, you have to write all of the data.  The reason 
I introduced a return value at all was so that you could return an 
error.  This is necessary to detect that a migration or save/restore has 
failed.  I agree the QEMUFile API isn't ideal ATM.  The whole buffered 
thing is a big hack.  I'm open to refactoring suggestions/patches.

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

I don't see a tab in subversion so perhaps I cleaned that up before 
committing :-)

Thanks for the review!

Regards,

Anthony Liguori

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