Re: [libvirt] [PATCH 1/2] Implementation deficiency in virInitctlSetRunLevel v4

On 12/20/2013 11:17 AM, Eric Blake wrote: Yuck. Rewriting this to use virFork() has exposed some nastiness in our existing library: 'virsh lxc-enter-namespace' and 'virt-login-shell' both use virFork() incorrectly, which is a sign that the interface itself is to blame. The fact that a child process must explicitly call _exit() if virFork() returns < 0 but set *pid to 0 is just nasty - it violates the normal expectation that a negative return has no more work to do. I still plan on posting a revised version of this patch, but it will be part of a bit bigger series that first tries to make virFork and virProcessWait friendlier. I spent some time learning about namespaces in the process - I can see why 'virsh lxc-enter-namespace' must fork twice (although setns() can join some namespaces on a per-thread basis, it cannot do so for the 'user namespace', so one fork is required to get to a single-threaded state since virsh is multithreaded; the second fork is required because setting the 'pid namespace' doesn't actually change the pid of the current process, it only designates that child processes will be in the new namespace). But why is 'virt-login-shell' double-forking? It is already single-threaded, and doesn't do anything after fork except wait on the child process, so a single fork after setting the 'pid namespace' ought to be sufficient. Or maybe _exit(127), which is more idiomatic for failures when fork() succeeds but exec() is not reached (see posix_spawn() for example). On the other hand, we may NEED to pass status on to the user, as I noticed in patch 2 that your callback function wants to distinguish between fatal errors and the simpler case of no initctl support; collapsing all non-zero status into failure loses our chance to report multiple bits of information. That, and we still need the fix for virDomainDeviceAttach using /dev only within the guest namespace. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org