11:55 p.m.
Note that 'virsh lxc-enter-namespace' must double-fork, for two
reasons: some namespaces can only be done from a single thread,
while virsh is multithreaded; and because virsh can be run in
batch mode where we must not corrupt the namespace of that
execution upon return from the subsidiary command.
When virt-login-shell was first written, it blindly copied from
'virsh lxc-enter-namespace', including the double-fork. But
neither of the reasons for double forking apply to
virt-login-shell (we are single-threaded, and we have nothing to
do after the child completes that would require us to preserve a
namespace), so we can simplify life by using a single fork.
In turn, this will make it easier for a future patch to pass the
child's exit status on to the invoking shell.
In flattening to a single fork, note that closing the fds must
be done after fork, because the parent process still needs to
use fds to control the virConnectPtr; meanwhile, chdir can be
done prior to forking (in fact, it's easier to report errors
on anything attempted before forking).
* tools/virt-login-shell.c (main): Single rather than double fork.
(virLoginShellFini): Delete, by inlining actions instead.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
tools/virt-login-shell.c | 116 ++++++++++++++++++-----------------------------
1 file changed, 43 insertions(+), 73 deletions(-)
diff --git a/tools/virt-login-shell.c b/tools/virt-login-shell.c
index e26b7a1..bc8394e 100644
--- a/tools/virt-login-shell.c
+++ b/tools/virt-login-shell.c
@@ -41,24 +41,8 @@
#include "vircommand.h"
#define VIR_FROM_THIS VIR_FROM_NONE
-static ssize_t nfdlist;
-static int *fdlist;
static const char *conf_file = SYSCONFDIR "/libvirt/virt-login-shell.conf";
-static void virLoginShellFini(virConnectPtr conn, virDomainPtr dom)
-{
- size_t i;
-
- for (i = 0; i < nfdlist; i++)
- VIR_FORCE_CLOSE(fdlist[i]);
- VIR_FREE(fdlist);
- nfdlist = 0;
- if (dom)
- virDomainFree(dom);
- if (conn)
- virConnectClose(conn);
-}
-
static int virLoginShellAllowedUser(virConfPtr conf,
const char *name,
gid_t *groups)
@@ -187,7 +171,6 @@ main(int argc, char **argv)
pid_t cpid;
int ret = EXIT_FAILURE;
int status;
- int status2;
uid_t uid = getuid();
gid_t gid = getgid();
char *name = NULL;
@@ -201,6 +184,10 @@ main(int argc, char **argv)
int longindex = -1;
int ngroups;
gid_t *groups = NULL;
+ ssize_t nfdlist = 0;
+ int *fdlist = NULL;
+ int openmax;
+ size_t i;
struct option opt[] = {
{"help", no_argument, NULL, 'h'},
@@ -298,6 +285,13 @@ main(int argc, char **argv)
}
}
+ openmax = sysconf(_SC_OPEN_MAX);
+ if (openmax < 0) {
+ virReportSystemError(errno, "%s",
+ _("sysconf(_SC_OPEN_MAX) failed"));
+ goto cleanup;
+ }
+
if ((nfdlist = virDomainLxcOpenNamespace(dom, &fdlist, 0)) < 0)
goto cleanup;
if (VIR_ALLOC(secmodel) < 0)
@@ -308,76 +302,52 @@ main(int argc, char **argv)
goto cleanup;
if (virDomainGetSecurityLabel(dom, seclabel) < 0)
goto cleanup;
+ if (virSetUIDGID(0, 0, NULL, 0) < 0)
+ goto cleanup;
+ if (virDomainLxcEnterSecurityLabel(secmodel, seclabel, NULL, 0) < 0)
+ goto cleanup;
+ if (nfdlist > 0 &&
+ virDomainLxcEnterNamespace(dom, nfdlist, fdlist, NULL, NULL, 0) < 0)
+ goto cleanup;
+ if (virSetUIDGID(uid, gid, groups, ngroups) < 0)
+ goto cleanup;
+ if (chdir(homedir) < 0) {
+ virReportSystemError(errno, _("Unable to chdir(%s)"), homedir);
+ goto cleanup;
+ }
- /* Fork once because we don't want to affect virt-login-shell's
- * namespace itself. FIXME: is this necessary?
- */
+ /* A fork is required to create new process in correct pid namespace. */
if ((cpid = virFork()) < 0)
goto cleanup;
if (cpid == 0) {
- pid_t ccpid;
-
- int openmax = sysconf(_SC_OPEN_MAX);
- int fd;
+ int tmpfd;
- if (virSetUIDGID(0, 0, NULL, 0) < 0)
- return EXIT_FAILURE;
-
- if (virDomainLxcEnterSecurityLabel(secmodel,
- seclabel,
- NULL,
- 0) < 0)
- return EXIT_FAILURE;
-
- if (nfdlist > 0) {
- if (virDomainLxcEnterNamespace(dom,
- nfdlist,
- fdlist,
- NULL,
- NULL,
- 0) < 0)
- return EXIT_FAILURE;
- }
-
- ret = virSetUIDGID(uid, gid, groups, ngroups);
- VIR_FREE(groups);
- if (ret < 0)
- return EXIT_FAILURE;
-
- if (openmax < 0) {
- virReportSystemError(errno, "%s",
- _("sysconf(_SC_OPEN_MAX) failed"));
- return EXIT_FAILURE;
- }
- for (fd = 3; fd < openmax; fd++) {
- int tmpfd = fd;
+ for (i = 3; i < openmax; i++) {
+ tmpfd = i;
VIR_MASS_CLOSE(tmpfd);
}
-
- /* A fork is required to create new process in correct pid
- * namespace. */
- if ((ccpid = virFork()) < 0)
+ if (execv(shargv[0], (char *const*) shargv) < 0) {
+ virReportSystemError(errno, _("Unable to exec shell %s"),
+ shargv[0]);
return EXIT_FAILURE;
-
- if (ccpid == 0) {
- if (chdir(homedir) < 0) {
- virReportSystemError(errno, _("Unable to chdir(%s)"),
homedir);
- return EXIT_FAILURE;
- }
- if (execv(shargv[0], (char *const*) shargv) < 0) {
- virReportSystemError(errno, _("Unable to exec shell %s"),
- shargv[0]);
- return EXIT_FAILURE;
- }
}
- return virProcessWait(ccpid, &status2);
+ return EXIT_SUCCESS;
}
- ret = virProcessWait(cpid, &status);
+
+ if (virProcessWait(cpid, &status) < 0)
+ goto cleanup;
+ ret = EXIT_SUCCESS;
cleanup:
+ for (i = 0; i < nfdlist; i++)
+ VIR_FORCE_CLOSE(fdlist[i]);
+ VIR_FREE(fdlist);
virConfFree(conf);
- virLoginShellFini(conn, dom);
+ if (dom)
+ virDomainFree(dom);
+ if (conn)
+ virConnectClose(conn);
virStringFreeList(shargv);
VIR_FREE(name);
VIR_FREE(homedir);
--
1.8.4.2