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

[Peter Maydell]

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).

>     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