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

[Michael S. Tsirkin]

On Mon, Jun 08, 2009 at 08:54:52PM +0300, 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?
> 
> > +        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?
> 
> > +    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?

Looking at the code some more, at least qemu_get_byte really seems to
expect a reliable read underneath, so it won't work.  Generally the fact
that qemu_get_byte returns 0 on error seems broken to me maybe we could
fix that and then have a single loop retrying reads. But that's for
another patch.

Acked-by: Michael S. Tsirkin <address@hidden>


-- 
MST