From mboxrd@z Thu Jan  1 00:00:00 1970
From: Uri Lublin <uri@il.qumranet.com>
Subject: Re: [PATCH] qemu: qemu_fopen_fd: differentiate between reader and
 writer user
Date: Thu, 16 Oct 2008 03:36:43 +0200
Message-ID: <48F69AAB.4010404@il.qumranet.com>
References: <1223829030-14962-1-git-send-email-uril@qumranet.com> <48F22BF1.3000608@redhat.com> <48F23D4D.2050709@codemonkey.ws> <48F23F42.10405@redhat.com> <48F277A0.8040407@codemonkey.ws> <48F2BA83.7000101@codemonkey.ws>
Mime-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Cc: Avi Kivity <avi@redhat.com>, Uri Lublin <uril@qumranet.com>,
	kvm@vger.kernel.org
To: Anthony Liguori <anthony@codemonkey.ws>
Return-path: <kvm-owner@vger.kernel.org>
Received: from il.qumranet.com ([212.179.150.194]:35568 "EHLO il.qumranet.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1753353AbYJPBgp (ORCPT <rfc822;kvm@vger.kernel.org>);
	Wed, 15 Oct 2008 21:36:45 -0400
In-Reply-To: <48F2BA83.7000101@codemonkey.ws>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

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