2:31 p.m.
On 04/04/2011 06:55 AM, 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.
How worried are we about the child not doing any async-unsafe actions if
it wishes to avoid deadlock? We've already previously identified this
as a bug (such as in our handling of malloc in the child), but it's
somewhat of a corner-case, and I'm not sure how invasive it will be to
fix properly; so I am okay if a fixed version of this patch goes in
while we still leave that larger issue open for thought.
+++ b/src/util/command.c
@@ -35,6 +35,11 @@
#include "files.h"
#include "buf.h"
+#include <stdlib.h>
+#include <stdbool.h>
"internal.h" guaranteed this one for us.
+#include <poll.h>
+#include <sys/wait.h>
Why do we need this one? We're already using WIFEXITED and friends
without explicitly needing this header.
@@ -1115,7 +1129,6 @@ virCommandRun(virCommandPtr cmd, int
*exitstatus)
return ret;
}
-
/*
* Perform all virCommand-specific actions, along with the user hook.
*/
Spurious whitespace change?
@@ -1125,12 +1138,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);
This adds yet more calls to malloc() inside the child,
+ }
if (res == 0 && cmd->pwd) {
VIR_DEBUG("Running child in %s", cmd->pwd);
...but no worse than what we've already been doing. If VIR_DEBUG were
the only use of malloc() in the child, then it could just boil down to
conditionally compiling VIR_DEBUG statements where they can be turned on
for debugging the implementation, but compiled out later to avoid
deadlock (at least, I'm assuming that VIR_DEBUG uses malloc() to format
a timestamp prior to the message when posting to the circular buffer).
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)) {
Since c is char, sizeof(c) == 1 by definition. But I'm okay with you
spelling out the longer form.
+ 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");
Hmm, now that's a lot more use of malloc(), and not just in VIR_DEBUG()
calls.
+
+void virCommandRequireHandshake(virCommandPtr cmd)
+{
+ if (pipe(cmd->handshakeWait) < 0) {
NULL dereference if cmd had a previous failure. You need to guard with:
if (!cmd || cmd->has_error) return;
+
+int virCommandHandshakeWait(virCommandPtr cmd)
+{
+ char c;
+ int rv;
+ VIR_DEBUG("Wait for handshake on %d", cmd->handshakeWait[0]);
Likewise.
+
+int virCommandHandshakeNotify(virCommandPtr cmd)
+{
+ char c = '1';
+ VIR_DEBUG("Notify handshake on %d", cmd->handshakeWait[0]);
Likewise.
+++ b/src/util/command.h
@@ -274,6 +274,11 @@ int virCommandRunAsync(virCommandPtr cmd,
int virCommandWait(virCommandPtr cmd,
int *exitstatus) ATTRIBUTE_RETURN_CHECK;
+void virCommandRequireHandshake(virCommandPtr cmd);
+
+int virCommandHandshakeWait(virCommandPtr cmd);
+int virCommandHandshakeNotify(virCommandPtr cmd);
These last two could use ATTRIBUTE_RETURN_CHECK. All three could use
documentation.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org