| Message ID | 1334777449-18542-3-git-send-email-lcapitulino@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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.
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);
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(-)