[libvirt] [PATCH 08/14] Add helper for running code in separate namespaces
Eric Blake
eblake at redhat.com
Fri Feb 7 17:31:03 UTC 2014
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>
More information about the libvir-list
mailing list