From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43)
	id 1ME0Vm-000712-Kz
	for qemu-devel@nongnu.org; Tue, 09 Jun 2009 08:33:06 -0400
Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43)
	id 1ME0Vi-0006xm-TW
	for qemu-devel@nongnu.org; Tue, 09 Jun 2009 08:33:06 -0400
Received: from [199.232.76.173] (port=50255 helo=monty-python.gnu.org)
	by lists.gnu.org with esmtp (Exim 4.43) id 1ME0Vi-0006xa-Nr
	for qemu-devel@nongnu.org; Tue, 09 Jun 2009 08:33:02 -0400
Received: from mx2.redhat.com ([66.187.237.31]:49934)
	by monty-python.gnu.org with esmtp (Exim 4.60)
	(envelope-from <uril@redhat.com>) id 1ME0Vi-0002aG-9l
	for qemu-devel@nongnu.org; Tue, 09 Jun 2009 08:33:02 -0400
Received: from int-mx2.corp.redhat.com (int-mx2.corp.redhat.com [172.16.27.26])
	by mx2.redhat.com (8.13.8/8.13.8) with ESMTP id n59CX1Yt029925
	for <qemu-devel@nongnu.org>; Tue, 9 Jun 2009 08:33:01 -0400
Message-ID: <4A2E5679.6050104@redhat.com>
Date: Tue, 09 Jun 2009 15:32:57 +0300
From: Uri Lublin <uril@redhat.com>
MIME-Version: 1.0
Subject: Re: [Qemu-devel] [PATCH] exec-migration: handle EINTR
	in	popen_get_buffer()
References: <1244478441-26288-1-git-send-email-uril@redhat.com>
	<20090608175452.GA4995@redhat.com>
In-Reply-To: <20090608175452.GA4995@redhat.com>
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Transfer-Encoding: 7bit
List-Id: qemu-devel.nongnu.org
List-Unsubscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.gnu.org/pipermail/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: "Michael S. Tsirkin" <mst@redhat.com>
Cc: qemu-devel@nongnu.org

On 06/08/2009 08:54 PM, Michael S. Tsirkin wrote:
> On Mon, Jun 08, 2009 at 07:27:21PM +0300, Uri Lublin wrote:
>> Sometimes, upon interrupt, fread returns with no data, and
>> the (incoming exec) migration fails.
>>
>> Fix by retrying on such a case.
>>
>> Signed-off-by: Uri Lublin<uril@redhat.com>
>> ---
>>   savevm.c |    9 ++++++++-
>>   1 files changed, 8 insertions(+), 1 deletions(-)
>>
>> diff --git a/savevm.c b/savevm.c
>> index 248aea3..df2486d 100644
>> --- a/savevm.c
>> +++ b/savevm.c
>> @@ -215,7 +215,14 @@ static int popen_put_buffer(void *opaque, const uint8_t *buf, int64_t pos, int s
>>   static int popen_get_buffer(void *opaque, uint8_t *buf, int64_t pos, int size)
>>   {
>>       QEMUFilePopen *s = opaque;
>> -    return fread(buf, 1, size, s->popen_file);
>> +    FILE *fp = s->popen_file;
>> +    int bytes;
>> +
>> +    do {
>> +        clearerr(fp);
>
> Would it make sense to only clearerr on EINTR - if we intend to retry?

I am not sure we should be worried about calling clearerr.
I guess we can clearerr only upon ferror()!=0.


>
>> +        bytes = fread(buf, 1, size, fp);
>> +    } while ((bytes == 0)&&  ferror(fp)&&  (errno == EINTR));
>
>
> This does nothing about partial reads (bytes != 0)
> I think it's intentional because the user actually retries
> partial reads. Right?

It is intentional. The caller takes care of partial reads (lazily calling 
fill_buffer when needed).

>
>> +    return bytes;
>>   }
>>
>>   static int popen_close(void *opaque)
>
> Looking at qemu_fill_buffer, it seems that it is enough to set
> bytes to -EAGAIN. User will then retry. Correct?
>

Currently qemu_fill_buffer does not retry (nor its callers).


Thanks for the review,
     Uri.