[libvirt] [PATCH] Allow handshake with child process during startup

[Eric Blake]

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 at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20110404/8b121559/attachment-0001.sig>