6:23 p.m.
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org