On 23.10.2012 16:51, Eric Blake wrote:
On 10/23/2012 03:39 AM, Michal Privoznik wrote:
> Currently, we use iohelper when saving/restoring a domain.
> However, if there's some kind of error (like I/O) it is not
> propagated to libvirt. Since it is not qemu who is doing
> the actual write() it will not get error. The iohelper does.
> Therefore we should check for iohelper errors as it makes
> libvirt more user friendly.
> ---
> src/libvirt_private.syms | 1 +
> src/qemu/qemu_driver.c | 7 ++++-
> src/util/virfile.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++
> src/util/virfile.h | 2 +
> 4 files changed, 55 insertions(+), 2 deletions(-)
I agree that catching iohelper errors is needed, but I'm worried that
you might introduce deadlock with this approach (then again, it may just
be theoretical):
> +++ b/src/qemu/qemu_driver.c
> @@ -2945,6 +2945,7 @@ endjob:
> if (rc < 0)
> VIR_WARN("Unable to resume guest CPUs after save
failure");
> }
> + virFileWrapperFdCatchError(wrapperFd);
> }
> if (qemuDomainObjEndAsyncJob(driver, vm) == 0)
> vm = NULL;
> @@ -3282,9 +3283,11 @@ doCoreDump(struct qemud_driver *driver,
>
> cleanup:
> VIR_FORCE_CLOSE(fd);
> - virFileWrapperFdFree(wrapperFd);
> - if (ret != 0)
> + if (ret != 0) {
> + virFileWrapperFdCatchError(wrapperFd);
> unlink(path);
You aren't doing any reading of the stderr file descriptor until you get
to the cleanup label. But suppose that iohelper encounters situations
that cause it to write more than PIPE_BUF bytes to stderr, but while
still continuing to run. Then, because no one is reading the other end
of the pipe, iohelper blocks on writes to stderr instead of proceeding
on with the rest of its operations, and thus never gets to a point where
it will exit in order to kick libvirtd over to the cleanup label to
start reading the stderr pipe in the first place.
> +++ b/src/util/virfile.c
> @@ -135,6 +135,7 @@ virFileDirectFdFlag(void)
> * read-write is not supported, just a single direction. */
> struct _virFileWrapperFd {
> virCommandPtr cmd; /* Child iohelper process to do the I/O. */
> + int err_fd; /* FD to read stderr of @cmd */
> };
>
> #ifndef WIN32
> @@ -229,6 +230,14 @@ virFileWrapperFdNew(int *fd, const char *name, unsigned int
flags)
> virCommandAddArg(ret->cmd, "0");
> }
>
> + /* In order to catch iohelper stderr, we must:
> + * - pass a FD to virCommand (-1 to auto-allocate one)
> + * - change iohelper's env so virLog functions print to stderr
> + */
> + ret->err_fd = -1;
> + virCommandSetErrorFD(ret->cmd, &ret->err_fd);
> + virCommandAddEnvPair(ret->cmd, "LIBVIRT_LOG_OUTPUTS",
"1:stderr");
Would it work to use virCommandSetErrorBuffer() instead, or does that
rely on using virCommandRun() to properly populate the buffer via
poll(), where we are stuck using virCommandRunAsync() instead? Or,
since we are already using virCommandRunAsync(), does that mean we need
to figure out how to incorporate a poll() to favorably handle stderr in
addition to everything else that iohelper is already handling?
Yeah, I've tried te virCommandSetErrorBuffer() way firstly. But it
requires the poll() which is wrapped in virCommandProcessIO(). However,
it is static so it would needed to be dig out and run in a separate
thread. But we can run virCommandRun directly then. So I think I can
incorporate our event loop. Just add a callback once ret->err_fd is
initialized.
Or am I worrying too much, and if iohelper hits any issue where it
would
be logging to stderr in the first place, it will be exiting before
filling up the stderr pipe and thus never deadlocking?
Well, with current implementation of iohelper there is only one place
where it doesn't die right after reporting an error:
src/util/iohelper.c:160
I don't know how possible is to hit that.
At any rate, depending on how that question is answered, this code does
indeed look useful.
Okay, since we would have to keep this limitation in mind when changing
iohelper (and honestly, I'll forget immediately after the push) I think
we should make this safer and read the error pipe in the event loop.
I'll update the patch and resend.
Michal