Patchwork [2/2] qemu-ga: suspend: handle EINTR

login
register
mail settings
Submitter Luiz Capitulino
Date April 18, 2012, 7:30 p.m.
Message ID <1334777449-18542-3-git-send-email-lcapitulino@redhat.com>
Download mbox | patch
Permalink /patch/153572/
State New
Headers show

Comments

Luiz Capitulino - April 18, 2012, 7:30 p.m.
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 <lcapitulino@redhat.com>
---
 qga/commands-posix.c |   12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)
Peter Maydell - April 18, 2012, 8:08 p.m.
On 18 April 2012 20:30, Luiz Capitulino <lcapitulino@redhat.com> 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 <lcapitulino@redhat.com>
> ---
>  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).

>     error_set(err, QERR_UNSUPPORTED);

Shouldn't we be setting QERR_UNDEFINED_ERROR if the read
fails for something other than EINTR? That's what we seem
to do for pipe() and fork() failure, anyway. (Or maybe they
should be setting QERR_UNSUPPORTED instead?)

-- PMM
Luiz Capitulino - April 18, 2012, 8:46 p.m.
On Wed, 18 Apr 2012 21:08:28 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:

> On 18 April 2012 20:30, Luiz Capitulino <lcapitulino@redhat.com> 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 <lcapitulino@redhat.com>
> > ---
> >  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.

Patch

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;
+        }
     }
 
     error_set(err, QERR_UNSUPPORTED);