Re: [Qemu-devel] [PATCH] exec-migration: handle EINTR in popen_get_buffer()

[Uri Lublin <uril@redhat.com>]

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.