On Wed, 18 Apr 2012 21:08:28 +0100
Peter Maydell <peter.mayd...@linaro.org> wrote:
> 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).
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.
