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@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