Re: [Qemu-devel] [PATCH 2/2] qemu-ga: suspend: handle EINTR

[Luiz Capitulino]

On Wed, 18 Apr 2012 21:08:28 +0100
Peter Maydell <address@hidden> wrote:

> On 18 April 2012 20:30, Luiz Capitulino <address@hidden> wrote:
> > The read() call in bios_supports_mode() can fail with EINTR if a child
> > terminates during the call. Handle it.
> >
> > Signed-off-by: Luiz Capitulino <address@hidden>
> > ---
> >  qga/commands-posix.c |   12 ++++++++----
> >  1 file changed, 8 insertions(+), 4 deletions(-)
> >
> > diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> > index 41ba0c5..4d8c067 100644
> > --- a/qga/commands-posix.c
> > +++ b/qga/commands-posix.c
> > @@ -621,10 +621,14 @@ static void bios_supports_mode(const char 
> > *pmutils_bin, const char *pmutils_arg,
> >         goto out;
> >     }
> >
> > -    ret = read(pipefds[0], &status, sizeof(status));
> > -    if (ret == sizeof(status) && WIFEXITED(status) &&
> > -        WEXITSTATUS(status) == SUSPEND_SUPPORTED) {
> > -            goto out;
> > +    while (true) {
> > +        ret = read(pipefds[0], &status, sizeof(status));
> > +        if (ret == sizeof(status) && WIFEXITED(status) &&
> > +            WEXITSTATUS(status) == SUSPEND_SUPPORTED) {
> > +                goto out;
> > +        } else if (ret == -1 && errno != EINTR) {
> > +            break;
> > +        }
> >     }
> 
> I think that if the child process terminates without writing
> to the pipe then this read() will always return 0 (end-of-file)
> and we'll loop forever... Rephrasing the loop as
>   do {
>      read
>      if (success condition) {
>         goto out;
>      }
>   } while (ret == -1 && errno == EINTR);
> 
> should fix that (as well as being clearer that we're only
> retrying on EINTR).

That's right. I mean, I think it's almost impossible to happen, but let's
do it the right way.

> 
> >     error_set(err, QERR_UNSUPPORTED);
> 
> Shouldn't we be setting QERR_UNDEFINED_ERROR if the read
> fails for something other than EINTR?

Right too.