On Thu, May 19, 2011 at 07:24:16AM -0400, Daniel P. Berrange wrote:
> Allow the parent process to perform a bi-directional handshake
> with the child process during fork/exec. The child process
> will fork and do its initial setup. Immediately prior to the
> exec(), it will stop & wait for a handshake from the parent
> process. The parent process will spawn the child and wait
> until the child reaches the handshake point. It will do
> whatever extra setup work is required, before signalling the
> child to continue.
>
> The implementation of this is done using two pairs of blocking
> pipes. The first pair is used to block the parent, until the
> child writes a single byte. Then the second pair pair is used
> to block the child, until the parent confirms with another
> single byte.
>
> * src/util/command.c, src/util/command.h,
> src/libvirt_private.syms: Add APIs to perform a handshake
> ---
> src/libvirt_private.syms | 3 +
> src/util/command.c | 182
> +++++++++++++++++++++++++++++++++++++++++++++-
> src/util/command.h | 22 ++++++
> 3 files changed, 206 insertions(+), 1 deletions(-)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 2f475b2..1b13c5c 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -109,11 +109,14 @@ virCommandClearCaps;
> virCommandDaemonize;
> virCommandExec;
> virCommandFree;
> +virCommandHandshakeNotify;
> +virCommandHandshakeWait;
> virCommandNew;
> virCommandNewArgList;
> virCommandNewArgs;
> virCommandNonblockingFDs;
> virCommandPreserveFD;
> +virCommandRequireHandshake;
> virCommandRun;
> virCommandRunAsync;
> virCommandSetErrorBuffer;
> diff --git a/src/util/command.c b/src/util/command.c
> index ebb90cb..2991daa 100644
> --- a/src/util/command.c
> +++ b/src/util/command.c
> @@ -36,6 +36,8 @@
> #include "files.h"
> #include "buf.h"
>
> +#include <stdlib.h>
> +
> #define VIR_FROM_THIS VIR_FROM_NONE
>
> #define virCommandError(code, ...) \
> @@ -77,6 +79,10 @@ struct _virCommand {
> int *outfdptr;
> int *errfdptr;
>
> + bool handshake;
> + int handshakeWait[2];
> + int handshakeNotify[2];
> +
> virExecHook hook;
> void *opaque;
>
> @@ -108,6 +114,11 @@ virCommandNewArgs(const char *const*args)
> if (VIR_ALLOC(cmd) < 0)
> return NULL;
>
> + cmd->handshakeWait[0] = -1;
> + cmd->handshakeWait[1] = -1;
> + cmd->handshakeNotify[0] = -1;
> + cmd->handshakeNotify[1] = -1;
> +
> FD_ZERO(&cmd->preserve);
> FD_ZERO(&cmd->transfer);
> cmd->infd = cmd->outfd = cmd->errfd = -1;
> @@ -1174,12 +1185,61 @@ virCommandHook(void *data)
> virCommandPtr cmd = data;
> int res = 0;
>
> - if (cmd->hook)
> + if (cmd->hook) {
> + VIR_DEBUG("Run hook %p %p", cmd->hook, cmd->opaque);
> res = cmd->hook(cmd->opaque);
> + VIR_DEBUG("Done hook %d", res);
> + }
> if (res == 0 && cmd->pwd) {
> VIR_DEBUG("Running child in %s", cmd->pwd);
> res = chdir(cmd->pwd);
> + if (res < 0) {
> + virReportSystemError(errno,
> + _("Unable to change to %s"), cmd->pwd);
> + }
> + }
> + if (cmd->handshake) {
> + char c = res < 0 ? '0' : '1';
> + int rv;
> + VIR_DEBUG("Notifying parent for handshake start on %d",
> cmd->handshakeWait[1]);
> + if (safewrite(cmd->handshakeWait[1], &c, sizeof(c)) != sizeof(c)) {
> + virReportSystemError(errno, "%s", _("Unable to notify parent
> process"));
> + return -1;
> + }
> +
> + /* On failure we pass the error message back to parent,
> + * so they don't have to dig through stderr logs
> + */
> + if (res < 0) {
> + virErrorPtr err = virGetLastError();
> + const char *msg = err ? err->message :
> + _("Unknown failure during hook execution");
> + size_t len = strlen(msg) + 1;
> + if (safewrite(cmd->handshakeWait[1], msg, len) != len) {
> + virReportSystemError(errno, "%s", _("Unable to send error to
> parent process"));
> + return -1;
> + }
> + return -1;
> + }
> +
> + VIR_DEBUG("Waiting on parent for handshake complete on %d",
> cmd->handshakeNotify[0]);
> + if ((rv = saferead(cmd->handshakeNotify[0], &c, sizeof(c))) !=
> sizeof(c)) {
> + if (rv < 0)
> + virReportSystemError(errno, "%s", _("Unable to wait on
> parent process"));
> + else
> + virReportSystemError(EIO, "%s", _("libvirtd quit during
> handshake"));
> + return -1;
> + }
> + if (c != '1') {
> + virReportSystemError(EINVAL, _("Unexpected confirm code '%c'
> from parent process"), c);
> + return -1;
> + }
> + VIR_FORCE_CLOSE(cmd->handshakeWait[1]);
> + VIR_FORCE_CLOSE(cmd->handshakeNotify[0]);
> }
> +
> + VIR_DEBUG("Hook is done %d", res);
> +
> return res;
> }
>
> @@ -1409,6 +1469,119 @@ virCommandAbort(virCommandPtr cmd ATTRIBUTE_UNUSED)
> }
> #endif
>
> +
> +void virCommandRequireHandshake(virCommandPtr cmd)
> +{
> + if (!cmd || cmd->has_error)
> + return;
> +
> + if (cmd->handshake) {
> + cmd->has_error = -1;
> + VIR_DEBUG("Cannot require handshake twice");
> + return;
> + }
> +
> + if (pipe(cmd->handshakeWait) < 0) {
> + cmd->has_error = errno;
> + return;
> + }
> + if (pipe(cmd->handshakeNotify) < 0) {
> + VIR_FORCE_CLOSE(cmd->handshakeWait[0]);
> + VIR_FORCE_CLOSE(cmd->handshakeWait[1]);
> + cmd->has_error = errno;
> + return;
> + }
> +
> + VIR_DEBUG("Transfer handshake wait=%d notify=%d",
> + cmd->handshakeWait[1], cmd->handshakeNotify[0]);
> + virCommandTransferFD(cmd, cmd->handshakeWait[1]);
> + virCommandTransferFD(cmd, cmd->handshakeNotify[0]);
> + cmd->handshake = true;
> +}
> +
> +int virCommandHandshakeWait(virCommandPtr cmd)
> +{
> + char c;
> + int rv;
> + if (!cmd ||cmd->has_error == ENOMEM) {
> + virReportOOMError();
> + return -1;
> + }
> + if (cmd->has_error || !cmd->handshake) {
> + virCommandError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("invalid use of command API"));
> + return -1;
> + }
> +
> + if (cmd->handshakeWait[0] == -1) {
> + virCommandError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Handshake is already complete"));
> + return -1;
> + }
> +
> + VIR_DEBUG("Wait for handshake on %d", cmd->handshakeWait[0]);
> + if ((rv = saferead(cmd->handshakeWait[0], &c, sizeof(c))) != sizeof(c)) {
> + if (rv < 0)
> + virReportSystemError(errno, "%s", _("Unable to wait for child
> process"));
> + else
> + virReportSystemError(EIO, "%s", _("Child process quit during
> startup handshake"));
> + VIR_FORCE_CLOSE(cmd->handshakeWait[0]);
> + return -1;
> + }
> + if (c != '1') {
> + char *msg;
> + ssize_t len;
> + if (VIR_ALLOC_N(msg, 1024) < 0) {
> + virReportOOMError();
> + VIR_FORCE_CLOSE(cmd->handshakeWait[0]);
> + return -1;
> + }
> + if ((len = saferead(cmd->handshakeWait[0], msg, 1024)) < 0) {
> + VIR_FORCE_CLOSE(cmd->handshakeWait[0]);
> + VIR_FREE(msg);
> + virReportSystemError(errno, "%s", _("No error message from child
> failure"));
> + return -1;
> + }
> + VIR_FORCE_CLOSE(cmd->handshakeWait[0]);
> + msg[len-1] = '\0';
> + virCommandError(VIR_ERR_INTERNAL_ERROR, "%s", msg);
> + VIR_FREE(msg);
> + return -1;
> + }
> + VIR_FORCE_CLOSE(cmd->handshakeWait[0]);
> + return 0;
> +}
> +
> +int virCommandHandshakeNotify(virCommandPtr cmd)
> +{
> + char c = '1';
> + if (!cmd ||cmd->has_error == ENOMEM) {
> + virReportOOMError();
> + return -1;
> + }
> + if (cmd->has_error || !cmd->handshake) {
> + virCommandError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("invalid use of command API"));
> + return -1;
> + }
> +
> + if (cmd->handshakeNotify[1] == -1) {
> + virCommandError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Handshake is already complete"));
> + return -1;
> + }
> +
> + VIR_DEBUG("Notify handshake on %d", cmd->handshakeWait[0]);
> + if (safewrite(cmd->handshakeNotify[1], &c, sizeof(c)) != sizeof(c)) {
> + virReportSystemError(errno, "%s", _("Unable to notify child
> process"));
> + VIR_FORCE_CLOSE(cmd->handshakeNotify[1]);
> + return -1;
> + }
> + VIR_FORCE_CLOSE(cmd->handshakeNotify[1]);
> + return 0;
> +}
> +
> +
> /*
> * Release all resources
> */
> @@ -1440,6 +1613,13 @@ virCommandFree(virCommandPtr cmd)
>
> VIR_FREE(cmd->pwd);
>
> + if (cmd->handshake) {
> + VIR_FORCE_CLOSE(cmd->handshakeWait[0]);
> + VIR_FORCE_CLOSE(cmd->handshakeWait[1]);
> + VIR_FORCE_CLOSE(cmd->handshakeNotify[0]);
> + VIR_FORCE_CLOSE(cmd->handshakeNotify[1]);
> + }
> +
> VIR_FREE(cmd->pidfile);
>
> if (cmd->reap)
> diff --git a/src/util/command.h b/src/util/command.h
> index aa5136b..95b6a5e 100644
> --- a/src/util/command.h
> +++ b/src/util/command.h
> @@ -292,6 +292,28 @@ int virCommandWait(virCommandPtr cmd,
> int *exitstatus) ATTRIBUTE_RETURN_CHECK;
>
> /*
> + * Request that the child perform a handshake with
> + * the parent when the hook function has completed
> + * execution. The child will not exec() until the
> + * parent has notified
> + */
> +void virCommandRequireHandshake(virCommandPtr cmd);
> +
> +/*
> + * Wait for the child to complete execution of its
> + * hook function
> + */
> +int virCommandHandshakeWait(virCommandPtr cmd)
> + ATTRIBUTE_RETURN_CHECK;
> +
> +/*
> + * Notify the child that it is OK to exec() the
> + * real binary now
> + */
> +int virCommandHandshakeNotify(virCommandPtr cmd)
> + ATTRIBUTE_RETURN_CHECK;
> +
> +/*
> * Abort an async command if it is running, without issuing
> * any errors or affecting errno. Designed for error paths
> * where some but not all paths to the cleanup code might
Looks fine to me, I was wondering if passing a changing value like
the LSB of the child pid would be of any interest, probably not we're
always operating from fork() there should not be any risk.
ACK,
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
dan...@veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
