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@xxxxxxxxx> at > http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=732394 > > Signed-off-by: Daniel Berrange <berrange@xxxxxxxxxx> Looks like you improved from my version, so it would be nice to add: Signed-off-by: Eric Blake <eblake@xxxxxxxxxx> > + > +/* 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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list