Re: [libvirt] PATCH: 5/5: Make all code use virExec

This final patch switches over all places which do fork()/exec() to use the new enhanced virExec() code. A fair amount of code is deleted, but that's not the whole story - the new impl is more robust that most of the existing code we're deleting here. bridge.c | 51 +++------------- proxy_internal.c | 25 +------- qemu_conf.c | 168 ++++++++++++++++++++++-------------------------------- remote_internal.c | 88 ++++------------------------ 4 files changed, 100 insertions(+), 232 deletions(-) Daniel diff -r 2591ebc40bd7 src/bridge.c --- a/src/bridge.c Tue Aug 12 15:29:29 2008 +0100 +++ b/src/bridge.c Tue Aug 12 15:33:42 2008 +0100 @@ -46,6 +46,7 @@ #include "internal.h" #include "memory.h" +#include "util.h" #define MAX_BRIDGE_ID 256 @@ -596,42 +597,6 @@ return brGetInetAddr(ctl, ifname, SIOCGIFNETMASK, addr, maxlen); } -static int -brctlSpawn(char * const *argv) -{ - pid_t pid, ret; - int status; - int null = -1; - - if ((null = open(_PATH_DEVNULL, O_RDONLY)) < 0) - return errno; - - pid = fork(); - if (pid == -1) { - int saved_errno = errno; - close(null); - return saved_errno; - } - - if (pid == 0) { /* child */ - dup2(null, STDIN_FILENO); - dup2(null, STDOUT_FILENO); - dup2(null, STDERR_FILENO); - close(null); - - execvp(argv[0], argv); - - _exit (1); - } - - close(null); - - while ((ret = waitpid(pid, &status, 0) == -1) && errno == EINTR); - if (ret == -1) - return errno; - - return (WIFEXITED(status) && WEXITSTATUS(status) == 0) ? 0 : EINVAL; -} /** * brSetForwardDelay: @@ -649,7 +614,7 @@ const char *bridge, int delay) { - char **argv; + const char **argv; int retval = ENOMEM; int n; char delayStr[30]; @@ -680,7 +645,10 @@ argv[n++] = NULL; - retval = brctlSpawn(argv); + if (virRun(NULL, argv, NULL) < 0) + retval = errno; + else + retval = 0; error: if (argv) { @@ -709,7 +677,7 @@ const char *bridge, int enable) { - char **argv; + const char **argv; int retval = ENOMEM; int n; @@ -737,7 +705,10 @@ argv[n++] = NULL; - retval = brctlSpawn(argv); + if (virRun(NULL, argv, NULL) < 0) + retval = errno; + else + retval = 0; error: if (argv) { diff -r 2591ebc40bd7 src/proxy_internal.c --- a/src/proxy_internal.c Tue Aug 12 15:29:29 2008 +0100 +++ b/src/proxy_internal.c Tue Aug 12 15:33:42 2008 +0100 @@ -162,6 +162,7 @@ { const char *proxyPath = virProxyFindServerPath(); int ret, pid, status; + const char *proxyarg[2]; if (!proxyPath) { fprintf(stderr, "failed to find libvirt_proxy\n"); @@ -171,27 +172,11 @@ if (debug) fprintf(stderr, "Asking to launch %s\n", proxyPath); - /* Become a daemon */ - pid = fork(); - if (pid == 0) { - long open_max; - long i; + proxyarg[0] = proxyPath; + proxyarg[1] = NULL; - /* don't hold open fd opened from the client of the library */ - open_max = sysconf (_SC_OPEN_MAX); - for (i = 0; i < open_max; i++) - fcntl (i, F_SETFD, FD_CLOEXEC); - - setsid(); - if (fork() == 0) { - execl(proxyPath, proxyPath, NULL); - fprintf(stderr, _("failed to exec %s\n"), proxyPath); - } - /* - * calling exit() generate troubles for termination handlers - */ - _exit(0); - } + if (virExec(NULL, proxyarg, NULL, &pid, -1, NULL, NULL, VIR_EXEC_DAEMON) < 0) + fprintf(stderr, "Failed to fork libvirt_proxy\n"); /* * do a waitpid on the intermediate process to avoid zombies. diff -r 2591ebc40bd7 src/qemu_conf.c --- a/src/qemu_conf.c Tue Aug 12 15:29:29 2008 +0100 +++ b/src/qemu_conf.c Tue Aug 12 15:33:42 2008 +0100 @@ -397,116 +397,88 @@ int qemudExtractVersionInfo(const char *qemu, int *version, int *flags) { + const char *const qemuarg[] = { qemu, "-help", NULL }; + const char *const qemuenv[] = { "LANG=C", NULL }; pid_t child; - int newstdout[2]; + int newstdout = -1; + char help[8192]; /* Ought to be enough to hold QEMU help screen */ + int got = 0, ret = -1, status; + int major, minor, micro; + int ver; if (flags) *flags = 0; if (version) *version = 0; - if (pipe(newstdout) < 0) { + if (virExec(NULL, qemuarg, qemuenv, &child, + -1, &newstdout, NULL, VIR_EXEC_NONE) < 0) return -1; + + + while (got < (sizeof(help)-1)) { + int len; + if ((len = read(newstdout, help+got, sizeof(help)-got-1)) <= 0) { + if (!len) + break; + if (errno == EINTR) + continue; + goto cleanup2; + } + got += len; + } + help[got] = '\0'; + + if (sscanf(help, "QEMU PC emulator version %d.%d.%d", &major,&minor, &micro) != 3) { + goto cleanup2; } - if ((child = fork()) < 0) { - close(newstdout[0]); - close(newstdout[1]); - return -1; + ver = (major * 1000 * 1000) + (minor * 1000) + micro; + if (version) + *version = ver; + if (flags) { + if (strstr(help, "-no-kqemu")) + *flags |= QEMUD_CMD_FLAG_KQEMU; + if (strstr(help, "-no-reboot")) + *flags |= QEMUD_CMD_FLAG_NO_REBOOT; + if (strstr(help, "-name")) + *flags |= QEMUD_CMD_FLAG_NAME; + if (strstr(help, "-drive")) + *flags |= QEMUD_CMD_FLAG_DRIVE; + if (strstr(help, "boot=on")) + *flags |= QEMUD_CMD_FLAG_DRIVE_BOOT; + if (ver >= 9000) + *flags |= QEMUD_CMD_FLAG_VNC_COLON; + } + ret = 0; + + qemudDebug("Version %d %d %d Cooked version: %d, with flags ? %d", + major, minor, micro, *version, *flags); + +cleanup2: + if (close(newstdout) < 0) + ret = -1; + +rewait: + if (waitpid(child, &status, 0) != child) { + if (errno == EINTR) + goto rewait; + + qemudLog(QEMUD_ERR, + _("Unexpected exit status from qemu %d pid %lu"), + status, (unsigned long)child); + ret = -1; + } + /* Check & log unexpected exit status, but don't fail, + * as there's really no need to throw an error if we did + * actually read a valid version number above */ + if (WEXITSTATUS(status) != 0) { + qemudLog(QEMUD_WARN, + _("Unexpected exit status '%d', qemu probably failed"), + status); } - if (child == 0) { /* Kid */ - /* Just in case QEMU is translated someday we force to C locale.. */ - const char *const qemuenv[] = { "LANG=C", NULL }; - - if (close(STDIN_FILENO) < 0) - goto cleanup1; - if (close(STDERR_FILENO) < 0) - goto cleanup1; - if (close(newstdout[0]) < 0) - goto cleanup1; - if (dup2(newstdout[1], STDOUT_FILENO) < 0) - goto cleanup1; - - /* Passing -help, rather than relying on no-args which doesn't - always work */ - execle(qemu, qemu, "-help", (char*)NULL, qemuenv); - - cleanup1: - _exit(-1); /* Just in case */ - } else { /* Parent */ - char help[8192]; /* Ought to be enough to hold QEMU help screen */ - int got = 0, ret = -1; - int major, minor, micro; - int ver; - - if (close(newstdout[1]) < 0) - goto cleanup2; - - while (got < (sizeof(help)-1)) { - int len; - if ((len = read(newstdout[0], help+got, sizeof(help)-got-1)) <= 0) { - if (!len) - break; - if (errno == EINTR) - continue; - goto cleanup2; - } - got += len; - } - help[got] = '\0'; - - if (sscanf(help, "QEMU PC emulator version %d.%d.%d", &major,&minor, &micro) != 3) { - goto cleanup2; - } - - ver = (major * 1000 * 1000) + (minor * 1000) + micro; - if (version) - *version = ver; - if (flags) { - if (strstr(help, "-no-kqemu")) - *flags |= QEMUD_CMD_FLAG_KQEMU; - if (strstr(help, "-no-reboot")) - *flags |= QEMUD_CMD_FLAG_NO_REBOOT; - if (strstr(help, "-name")) - *flags |= QEMUD_CMD_FLAG_NAME; - if (strstr(help, "-drive")) - *flags |= QEMUD_CMD_FLAG_DRIVE; - if (strstr(help, "boot=on")) - *flags |= QEMUD_CMD_FLAG_DRIVE_BOOT; - if (ver >= 9000) - *flags |= QEMUD_CMD_FLAG_VNC_COLON; - } - ret = 0; - - qemudDebug("Version %d %d %d Cooked version: %d, with flags ? %d", - major, minor, micro, *version, *flags); - - cleanup2: - if (close(newstdout[0]) < 0) - ret = -1; - - rewait: - if (waitpid(child, &got, 0) != child) { - if (errno == EINTR) { - goto rewait; - } - qemudLog(QEMUD_ERR, - _("Unexpected exit status from qemu %d pid %lu"), - got, (unsigned long)child); - ret = -1; - } - /* Check & log unexpected exit status, but don't fail, - * as there's really no need to throw an error if we did - * actually read a valid version number above */ - if (WEXITSTATUS(got) != 0) { - qemudLog(QEMUD_WARN, - _("Unexpected exit status '%d', qemu probably failed"), - got); - } - - return ret; - } + return ret; } int qemudExtractVersion(virConnectPtr conn, diff -r 2591ebc40bd7 src/remote_internal.c --- a/src/remote_internal.c Tue Aug 12 15:29:29 2008 +0100 +++ b/src/remote_internal.c Tue Aug 12 15:33:42 2008 +0100 @@ -72,6 +72,7 @@ #include "remote_internal.h" #include "remote_protocol.h" #include "memory.h" +#include "util.h" #define DEBUG(fmt,...) VIR_DEBUG(__FILE__, fmt,__VA_ARGS__) #define DEBUG0(msg) VIR_DEBUG(__FILE__, "%s", msg) @@ -221,61 +222,21 @@ remoteForkDaemon(virConnectPtr conn) { const char *daemonPath = remoteFindDaemonPath(); + const char *daemonargs[4]; int ret, pid, status; + + daemonargs[0] = daemonPath; + daemonargs[1] = "--timeout"; + daemonargs[2] = "30"; + daemonargs[3] = NULL; if (!daemonPath) { error(conn, VIR_ERR_INTERNAL_ERROR, _("failed to find libvirtd binary")); return(-1); } - /* Become a daemon */ - pid = fork(); - if (pid == 0) { - int stdinfd = -1; - int stdoutfd = -1; - int i, open_max; - if ((stdinfd = open(_PATH_DEVNULL, O_RDONLY)) < 0) - goto cleanup; - if ((stdoutfd = open(_PATH_DEVNULL, O_WRONLY)) < 0) - goto cleanup; - if (dup2(stdinfd, STDIN_FILENO) != STDIN_FILENO) - goto cleanup; - if (dup2(stdoutfd, STDOUT_FILENO) != STDOUT_FILENO) - goto cleanup; - if (dup2(stdoutfd, STDERR_FILENO) != STDERR_FILENO) - goto cleanup; - if (close(stdinfd) < 0) - goto cleanup; - stdinfd = -1; - if (close(stdoutfd) < 0) - goto cleanup; - stdoutfd = -1; - - open_max = sysconf (_SC_OPEN_MAX); - for (i = 0; i < open_max; i++) - if (i != STDIN_FILENO && - i != STDOUT_FILENO && - i != STDERR_FILENO) - close(i); - - setsid(); - if (fork() == 0) { - /* Run daemon in auto-shutdown mode, so it goes away when - no longer needed by an active guest, or client */ - execl(daemonPath, daemonPath, "--timeout", "30", NULL); - } - /* - * calling exit() generate troubles for termination handlers - */ - _exit(0); - - cleanup: - if (stdoutfd != -1) - close(stdoutfd); - if (stdinfd != -1) - close(stdinfd); - _exit(-1); - } + if (virExec(NULL, daemonargs, NULL, &pid, -1, NULL, NULL, VIR_EXEC_DAEMON) < 0) + error(conn, VIR_ERR_INTERNAL_ERROR, _("failed to fork libvirtd binary")); /* * do a waitpid on the intermediate process to avoid zombies. @@ -349,7 +310,7 @@ char *name = 0, *command = 0, *sockname = 0, *netcat = 0, *username = 0; char *port = 0, *authtype = 0; int no_verify = 0, no_tty = 0; - char **cmd_argv = 0; + char **cmd_argv = NULL; /* Return code from this function, and the private data. */ int retcode = VIR_DRV_OPEN_ERROR; @@ -689,31 +650,10 @@ goto failed; } - pid = fork (); - if (pid == -1) { - error (conn, VIR_ERR_SYSTEM_ERROR, strerror (errno)); - goto failed; - } else if (pid == 0) { /* Child. */ - close (sv[0]); - // Connect socket (sv[1]) to stdin/stdout. - close (0); - if (dup (sv[1]) == -1) perror ("dup"); - close (1); - if (dup (sv[1]) == -1) perror ("dup"); - close (sv[1]); - - // Run the external process. - if (!cmd_argv) { - if (VIR_ALLOC_N(cmd_argv, 2) < 0) { - error (conn, VIR_ERR_SYSTEM_ERROR, strerror (errno)); - goto failed; - } - cmd_argv[0] = command; - cmd_argv[1] = 0; - } - execvp (command, cmd_argv); - perror (command); - _exit (1); + if (virExec(conn, (const char**)cmd_argv, NULL, + &pid, sv[1], &(sv[1]), NULL, VIR_EXEC_NONE) < 0) { + error (conn, VIR_ERR_SYSTEM_ERROR, strerror (errno)); + goto failed; } /* Parent continues here. */ -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|