On 11/22/2010 11:09 AM, Daniel P. Berrange wrote:
[reviving this thread a bit]
> 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 | 92 ++++++++++++++++++++++++++++++++++++++++++++--
> src/util/command.h | 5 ++
> 3 files changed, 96 insertions(+), 4 deletions(-)
>
> @@ -626,8 +632,8 @@ int virCommandRun(virCommandPtr cmd,
> ret = -1;
>
> VIR_DEBUG("Result stdout: '%s' stderr: '%s'",
> - NULLSTR(*cmd->outbuf),
> - NULLSTR(*cmd->errbuf));
> + cmd->outbuf ? NULLSTR(*cmd->outbuf) : "(null)",
> + cmd->errbuf ? NULLSTR(*cmd->errbuf) : "(null)");
Ah - I see you stumbled on the same problem that has already been fixed.
You won't need this hunk :)
>
> +static int virCommandHookImpl(void *data)
> +{
Hmm, I already implemented a static function virCommandHook() for
managing cwd swapping; this should be merged into that function.
> + virCommandPtr cmd = data;
> +
> + if (cmd->hook &&
> + cmd->hook(cmd->opaque) < 0)
> + return -1;
> +
> + if (cmd->handshake) {
> + char c = 'w';
> + VIR_WARN0("Notifying parent for handshake start");
> + if (safewrite(cmd->handshakeWait[1], &c, sizeof(c)) != sizeof(c)) {
> + virReportSystemError(errno, "%s", _("Unable to notify parent process"));
> + return -1;
> + }
> + VIR_WARN0("Waiting on parent for handshake complete ");
trailing space
> + if (saferead(cmd->handshakeNotify[0], &c, sizeof(c)) != sizeof(c)) {
> + virReportSystemError(errno, "%s", _("Unable to wait on parent process"));
> + return -1;
> + }
> + if (c != 'n') {
> + virReportSystemError(EINVAL, _("Unexpected data '%d' from parent process"), (int)c);
> + return -1;
> + }
> + VIR_WARN0("Handshake is done, child is running");
> + VIR_FORCE_CLOSE(cmd->handshakeWait[1]);
> + VIR_FORCE_CLOSE(cmd->handshakeNotify[0]);
> + }
> +
> + return 0;
> +}
Looks correct for the hook.
>
>
> +void virCommandRequireHandshake(virCommandPtr cmd)
> +{
if (!cmd || cmd->has_error) return;
> + if (pipe(cmd->handshakeWait) < 0)
> + cmd->has_error = errno;
> + if (pipe(cmd->handshakeNotify) < 0) {
> + VIR_FORCE_CLOSE(cmd->handshakeWait[0]);
> + VIR_FORCE_CLOSE(cmd->handshakeWait[1]);
> + }
Oops; forgot to set cmd->has_error in this case. And this is the first
instance of setting cmd->has_error to an errno other than ENOMEM; we'll
have to make sure virCommandRun() does the right thing in this case.
Also, we should have a sanity check that use of RequireHandshake entails
that virCommandRunAsync (and not virCommandRun) is called, so that we
don't deadlock. Can RequireHandshake and Daemonize be mixed, or should
our sanity checks declare them as incompatible?
> +
> + virCommandPreserveFD(cmd, cmd->handshakeWait[1]);
> + virCommandPreserveFD(cmd, cmd->handshakeNotify[0]);
Would these be better as virCommandTransferFD()?
> + cmd->handshake = true;
> +}
> +
> +int virCommandHandshakeWait(virCommandPtr cmd)
> +{
> + char c;
Needs error checking that the command has been started but not yet
waited on (that is, cmd->pid should be non-negative).
> + if (saferead(cmd->handshakeWait[0], &c, sizeof(c)) != sizeof(c)) {
> + virReportSystemError(errno, "%s", _("Unable to wait for child process"));
> + return -1;
> + }
> + if (c != 'w') {
> + virReportSystemError(EINVAL, _("Unexpected data '%d' from child process"), (int)c);
> + return -1;
> + }
> + return 0;
> +}
> +
> +int virCommandHandshakeNotify(virCommandPtr cmd)
> +{
> + char c = 'n';
Likewise. Should this also check that handshakewait has been called first?
> + if (safewrite(cmd->handshakeNotify[1], &c, sizeof(c)) != sizeof(c)) {
> + virReportSystemError(errno, "%s", _("Unable to notify child process"));
> + return -1;
> + }
> + return 0;
> +}
> +
> +
> +void virCommandRequireHandshake(virCommandPtr cmd);
> +
> +int virCommandHandshakeWait(virCommandPtr cmd);
> +int virCommandHandshakeNotify(virCommandPtr cmd);
Should these last two be marked ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK?
Do you want me to take over modernizing this patch, and updating
tests/commandtest.c to exercise it, since I've been doing other
virCommand code?
--
Eric Blake eblake@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list