From 3348a97bd35299dafe75b3df7d8060fde6d9799c Mon Sep 17 00:00:00 2001 From: "Daniel P. Berrange" Date: Fri, 29 Aug 2008 07:11:15 +0000 Subject: [PATCH] Switch over fork/exec code to use virExec --- ChangeLog | 6 ++ src/bridge.c | 128 ++++------------------------ src/proxy_internal.c | 26 ++---- src/qemu_conf.c | 188 ++++++++++++++++++------------------------ src/qemu_conf.h | 8 +- src/qemu_driver.c | 15 +--- src/remote_internal.c | 95 +++------------------ 7 files changed, 128 insertions(+), 338 deletions(-) diff --git a/ChangeLog b/ChangeLog index 857b18fd2b..fbdf485521 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,9 @@ +Fri Aug 29 08:04:15 BST 2008 Daniel P. Berrange + + * src/bridge.c, src/proxy_internal.c, src/qemu_conf.c, + src/qemu_conf.h, src/qemu_driver.c, src/remote_internal.c: + Switch over to using virExec() function + Thu Aug 28 23:39:15 BST 2008 Daniel P. Berrange * src/util.c: Fix off-by-one to allow making of paths at root diff --git a/src/bridge.c b/src/bridge.c index f78b471f7a..4090163f96 100644 --- a/src/bridge.c +++ b/src/bridge.c @@ -46,6 +46,7 @@ #include "internal.h" #include "memory.h" +#include "util.h" #define MAX_BRIDGE_ID 256 @@ -596,42 +597,6 @@ brGetInetNetmask(brControl *ctl, 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: @@ -641,7 +606,7 @@ brctlSpawn(char * const *argv) * * Set the bridge forward delay * - * Returns 0 in case of success or an errno code in case of failure. + * Returns 0 in case of success or -1 on failure */ int @@ -649,48 +614,17 @@ brSetForwardDelay(brControl *ctl ATTRIBUTE_UNUSED, const char *bridge, int delay) { - char **argv; - int retval = ENOMEM; - int n; char delayStr[30]; - - n = 1 + /* brctl */ - 1 + /* setfd */ - 1 + /* brige name */ - 1; /* value */ + const char *const progargv[] = { + BRCTL, "setfd", bridge, delayStr, NULL + }; snprintf(delayStr, sizeof(delayStr), "%d", delay); - if (VIR_ALLOC_N(argv, n + 1) < 0) - goto error; + if (virRun(NULL, progargv, NULL) < 0) + return -1; - n = 0; - - if (!(argv[n++] = strdup(BRCTL))) - goto error; - - if (!(argv[n++] = strdup("setfd"))) - goto error; - - if (!(argv[n++] = strdup(bridge))) - goto error; - - if (!(argv[n++] = strdup(delayStr))) - goto error; - - argv[n++] = NULL; - - retval = brctlSpawn(argv); - - error: - if (argv) { - n = 0; - while (argv[n]) - VIR_FREE(argv[n++]); - VIR_FREE(argv); - } - - return retval; + return 0; } /** @@ -702,52 +636,22 @@ brSetForwardDelay(brControl *ctl ATTRIBUTE_UNUSED, * Control whether the bridge participates in the spanning tree protocol, * in general don't disable it without good reasons. * - * Returns 0 in case of success or an errno code in case of failure. + * Returns 0 in case of success or -1 on failure */ int brSetEnableSTP(brControl *ctl ATTRIBUTE_UNUSED, const char *bridge, int enable) { - char **argv; - int retval = ENOMEM; - int n; + const char *setting = enable ? "on" : "off"; + const char *const progargv[] = { + BRCTL, "stp", bridge, setting, NULL + }; - n = 1 + /* brctl */ - 1 + /* stp */ - 1 + /* brige name */ - 1; /* value */ + if (virRun(NULL, progargv, NULL) < 0) + return -1; - if (VIR_ALLOC_N(argv, n + 1) < 0) - goto error; - - n = 0; - - if (!(argv[n++] = strdup(BRCTL))) - goto error; - - if (!(argv[n++] = strdup("stp"))) - goto error; - - if (!(argv[n++] = strdup(bridge))) - goto error; - - if (!(argv[n++] = strdup(enable ? "on" : "off"))) - goto error; - - argv[n++] = NULL; - - retval = brctlSpawn(argv); - - error: - if (argv) { - n = 0; - while (argv[n]) - VIR_FREE(argv[n++]); - VIR_FREE(argv); - } - - return retval; + return 0; } #endif /* WITH_QEMU || WITH_LXC */ diff --git a/src/proxy_internal.c b/src/proxy_internal.c index 8ed3342f99..13785598bc 100644 --- a/src/proxy_internal.c +++ b/src/proxy_internal.c @@ -160,6 +160,7 @@ virProxyForkServer(void) { const char *proxyPath = virProxyFindServerPath(); int ret, pid, status; + const char *proxyarg[2]; if (!proxyPath) { fprintf(stderr, "failed to find libvirt_proxy\n"); @@ -169,27 +170,12 @@ virProxyForkServer(void) 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, 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 --git a/src/qemu_conf.c b/src/qemu_conf.c index ee5851dc3b..bab2092747 100644 --- a/src/qemu_conf.c +++ b/src/qemu_conf.c @@ -394,124 +394,100 @@ virCapsPtr qemudCapsInit(void) { } -int qemudExtractVersionInfo(const char *qemu, int *version, int *flags) { +int qemudExtractVersionInfo(const char *qemu, + unsigned int *retversion, + unsigned int *retflags) { + const char *const qemuarg[] = { qemu, "-help", NULL }; + const char *const qemuenv[] = { "LC_ALL=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; + unsigned int major, minor, micro; + unsigned int version; + unsigned int flags = 0; - if (flags) - *flags = 0; - if (version) - *version = 0; + if (retflags) + *retflags = 0; + if (retversion) + *retversion = 0; - if (pipe(newstdout) < 0) { + if (virExec(NULL, qemuarg, qemuenv, NULL, + &child, -1, &newstdout, NULL, VIR_EXEC_NONE) < 0) return -1; - } - if ((child = fork()) < 0) { - close(newstdout[0]); - close(newstdout[1]); - return -1; - } - 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) + while (got < (sizeof(help)-1)) { + int len; + if ((len = saferead(newstdout, help+got, sizeof(help)-got-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, µ) != 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; + if (!len) + break; + got += len; } + help[got] = '\0'; + + if (sscanf(help, "QEMU PC emulator version %u.%u.%u", + &major, &minor, µ) != 3) { + goto cleanup2; + } + + version = (major * 1000 * 1000) + (minor * 1000) + micro; + + 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 (version >= 9000) + flags |= QEMUD_CMD_FLAG_VNC_COLON; + + + if (retversion) + *retversion = version; + if (retflags) + *retflags = flags; + + 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"), + WEXITSTATUS(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"), + WEXITSTATUS(status)); + } + + return ret; } int qemudExtractVersion(virConnectPtr conn, struct qemud_driver *driver) { const char *binary; struct stat sb; - int ignored; if (driver->qemuVersion > 0) return 0; @@ -529,7 +505,7 @@ int qemudExtractVersion(virConnectPtr conn, return -1; } - if (qemudExtractVersionInfo(binary, &driver->qemuVersion, &ignored) < 0) { + if (qemudExtractVersionInfo(binary, &driver->qemuVersion, NULL) < 0) { return -1; } @@ -716,7 +692,7 @@ static int qemudBuildCommandLineChrDevStr(virDomainChrDefPtr dev, int qemudBuildCommandLine(virConnectPtr conn, struct qemud_driver *driver, virDomainObjPtr vm, - int qemuCmdFlags, + unsigned int qemuCmdFlags, const char ***retargv, int **tapfds, int *ntapfds, diff --git a/src/qemu_conf.h b/src/qemu_conf.h index 96b361139d..88dfaded9d 100644 --- a/src/qemu_conf.h +++ b/src/qemu_conf.h @@ -49,7 +49,7 @@ enum qemud_cmd_flags { /* Main driver state */ struct qemud_driver { - int qemuVersion; + unsigned int qemuVersion; int nextvmid; virDomainObjPtr domains; @@ -86,13 +86,13 @@ virCapsPtr qemudCapsInit (void); int qemudExtractVersion (virConnectPtr conn, struct qemud_driver *driver); int qemudExtractVersionInfo (const char *qemu, - int *version, - int *flags); + unsigned int *version, + unsigned int *flags); int qemudBuildCommandLine (virConnectPtr conn, struct qemud_driver *driver, virDomainObjPtr dom, - int qemuCmdFlags, + unsigned int qemuCmdFlags, const char ***argv, int **tapfds, int *ntapfds, diff --git a/src/qemu_driver.c b/src/qemu_driver.c index 83a2eecf76..eb4454a5a5 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -312,6 +312,7 @@ static int qemudActive(void) { virDomainObjPtr dom = qemu_driver->domains; virNetworkObjPtr net = qemu_driver->networks; + while (dom) { if (virDomainIsActive(dom)) return 1; @@ -846,7 +847,7 @@ static int qemudStartVMDaemon(virConnectPtr conn, struct stat sb; int *tapfds = NULL; int ntapfds = 0; - int qemuCmdFlags; + unsigned int qemuCmdFlags; fd_set keepfd; FD_ZERO(&keepfd); @@ -1509,19 +1510,11 @@ static int qemudStartNetworkDaemon(virConnectPtr conn, } - if ((err = brSetForwardDelay(driver->brctl, network->def->bridge, network->def->delay))) { - qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, - _("failed to set bridge forward delay to %ld"), - network->def->delay); + if (brSetForwardDelay(driver->brctl, network->def->bridge, network->def->delay) < 0) goto err_delbr; - } - if ((err = brSetEnableSTP(driver->brctl, network->def->bridge, network->def->stp ? 1 : 0))) { - qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, - _("failed to set bridge STP to %s"), - network->def->stp ? "on" : "off"); + if (brSetEnableSTP(driver->brctl, network->def->bridge, network->def->stp ? 1 : 0) < 0) goto err_delbr; - } if (network->def->ipAddress && (err = brSetInetAddress(driver->brctl, network->def->bridge, network->def->ipAddress))) { diff --git a/src/remote_internal.c b/src/remote_internal.c index 586b34366e..c554303dea 100644 --- a/src/remote_internal.c +++ b/src/remote_internal.c @@ -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,62 +222,17 @@ static int remoteForkDaemon(virConnectPtr conn) { const char *daemonPath = remoteFindDaemonPath(); + const char *const daemonargs[] = { daemonPath, "--timeout=30", NULL }; int ret, pid, status; 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); + return -1; } + if (virExec(NULL, daemonargs, NULL, NULL, + &pid, -1, NULL, NULL, VIR_EXEC_DAEMON) < 0) + return -1; /* * do a waitpid on the intermediate process to avoid zombies. */ @@ -287,7 +243,7 @@ remoteForkDaemon(virConnectPtr conn) goto retry_wait; } - return (0); + return 0; } #endif @@ -349,7 +305,7 @@ doRemoteOpen (virConnectPtr conn, 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; @@ -693,40 +649,9 @@ doRemoteOpen (virConnectPtr conn, goto failed; } - pid = fork (); - if (pid == -1) { - errorf (conn, VIR_ERR_SYSTEM_ERROR, - _("unable to fork external network transport: %s"), - strerror (errno)); + if (virExec(conn, (const char**)cmd_argv, NULL, NULL, + &pid, sv[1], &(sv[1]), NULL, VIR_EXEC_NONE) < 0) 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"); - _exit(1); - } - close (1); - if (dup (sv[1]) == -1) { - perror ("dup"); - _exit(1); - } - close (sv[1]); - - // Run the external process. - if (!cmd_argv) { - if (VIR_ALLOC_N(cmd_argv, 2) < 0) { - perror("malloc"); - _exit(1); - } - cmd_argv[0] = command; - cmd_argv[1] = 0; - } - execvp (command, cmd_argv); - perror (command); - _exit (1); - } /* Parent continues here. */ close (sv[1]);