[libvirt] [PATCH 08/14] Add helper for running code in separate namespaces

[Eric Blake]

On 02/07/2014 08:33 AM, Daniel P. Berrange wrote:
> Implement virProcessRunInMountNamespace, which runs callback of type
> virProcessNamespaceCallback in a container namespace. This uses a
> child process to run the callback, since you can't change the mount
> namespace of a thread. This implies that callbacks have to be careful
> about what code they run due to async safety rules.
> 
> Idea by Dan Berrange, based on an initial report by Reco
> <recoverym4n at gmail.com> at
> http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=732394
> 
> Signed-off-by: Daniel Berrange <berrange at redhat.com>

Looks like you improved from my version, so it would be nice to add:

Signed-off-by: Eric Blake <eblake at redhat.com>

> +
> +/* Run cb(opaque) in the mount namespace of pid.  Return -1 with error
> + * message raised if we fail to run the child, if the child dies from
> + * a signal, or if the child has status EXIT_CANCELED; otherwise

Comment is wrong, since EXIT_CANCELED is not defined yet (it will be
fixed when I rebase the virFork cleanups on top of this).


> +    if (ret < 0 || child < 0) {
> +        if (child == 0)
> +            _exit(1);

Similarly, I prefer EXIT_FAILURE over the magic '1'. But I don't mind
cleaning that up in my rebased virFork stuff.

> +        else if (child > 0)

The else is redundant, since _exit() never returns.

> +    if (child == 0) {
> +        VIR_FORCE_CLOSE(errfd[0]);
> +        ret = virProcessNamespaceHelper(errfd[1], pid,
> +                                        cb, opaque);
> +        VIR_FORCE_CLOSE(errfd[1]);
> +        _exit(ret < 0 ? 1 : 0);

Again, more magic numbers I can clean up later.

> +
> +        ignore_value(virFileReadHeaderFD(errfd[0], 1024, &buf));
> +        if (virProcessWait(child, &status) < 0)
> +            ret = -1;
> +        else
> +            ret = status == 0 ? 0 : -1;

This can be simplified:

ret = virProcessWait(child, NULL);

> +        if (ret < 0)
> +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                           buf ? buf : _("Failed to run callback in mount namespace"));

And this error message overwrites the error from virProcessWait.

> +/* Callback to run code within the mount namespace tied to the given
> + * pid.  This function must use only async-signal-safe functions, as
> + * it gets run after a fork of a multi-threaded process.  The return
> + * value of this function is passed to _exit(), except that a
> + * negative value is treated as EXIT_CANCELED.  */
> +typedef int (*virProcessNamespaceCallback)(pid_t pid, void *opaque);

Again, the comment doesn't quite work with the rearranged patch order.

My comments above are improvements, but I didn't spot any bugs in your
implementation.

ACK.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

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