[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] exec-migration: handle EINTR in popen_get_buffe
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [PATCH] exec-migration: handle EINTR in popen_get_buffer() |
Date: |
Tue, 9 Jun 2009 15:20:57 +0300 |
User-agent: |
Mutt/1.5.18 (2008-05-17) |
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