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