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

[Uri Lublin]

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<address@hidden>
> > ---
> >   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.