On Wed, Aug 20, 2008 at 06:40:37PM +0200, Jim Meyering wrote:
> "Daniel P. Berrange" <berrange@xxxxxxxxxx> wrote:
> > 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.
>
> Nice clean-up!
I believe I've addressed all the comments you had - its quite a significant
change from the previous patch. I was in fact able to simplify the bridge.c
file even more than I did before.
bridge.c | 128 ++++--------------------------------
proxy_internal.c | 26 +------
qemu_conf.c | 188 +++++++++++++++++++++++-------------------------------
qemu_conf.h | 8 +-
qemu_driver.c | 15 +---
remote_internal.c | 101 +++--------------------------
6 files changed, 125 insertions(+), 341 deletions(-)
Daniel
diff -r c3d345142e1b src/bridge.c
--- a/src/bridge.c Wed Aug 27 12:45:11 2008 +0100
+++ b/src/bridge.c Wed Aug 27 14:09:26 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:
@@ -641,7 +606,7 @@
*
* 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 @@
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 @@
* 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 -r c3d345142e1b src/proxy_internal.c
--- a/src/proxy_internal.c Wed Aug 27 12:45:11 2008 +0100
+++ b/src/proxy_internal.c Wed Aug 27 14:09:26 2008 +0100
@@ -160,6 +160,7 @@
{
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 @@
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 -r c3d345142e1b src/qemu_conf.c
--- a/src/qemu_conf.c Wed Aug 27 12:45:11 2008 +0100
+++ b/src/qemu_conf.c Wed Aug 27 14:09:26 2008 +0100
@@ -394,124 +394,100 @@
}
-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;
+
+
+ while (got < (sizeof(help)-1)) {
+ int len;
+ if ((len = saferead(newstdout, help+got, sizeof(help)-got-1)) < 0)
+ goto cleanup2;
+ if (!len)
+ break;
+ got += len;
+ }
+ help[got] = '\0';
+
+ if (sscanf(help, "QEMU PC emulator version %u.%u.%u",
+ &major, &minor, µ) != 3) {
+ goto cleanup2;
}
- if ((child = fork()) < 0) {
- close(newstdout[0]);
- close(newstdout[1]);
- return -1;
+ 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));
}
- 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, µ) != 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,
struct qemud_driver *driver) {
const char *binary;
struct stat sb;
- int ignored;
if (driver->qemuVersion > 0)
return 0;
@@ -529,7 +505,7 @@
return -1;
}
- if (qemudExtractVersionInfo(binary, &driver->qemuVersion, &ignored) < 0) {
+ if (qemudExtractVersionInfo(binary, &driver->qemuVersion, NULL) < 0) {
return -1;
}
@@ -716,7 +692,7 @@
int qemudBuildCommandLine(virConnectPtr conn,
struct qemud_driver *driver,
virDomainObjPtr vm,
- int qemuCmdFlags,
+ unsigned int qemuCmdFlags,
const char ***retargv,
int **tapfds,
int *ntapfds,
diff -r c3d345142e1b src/qemu_conf.h
--- a/src/qemu_conf.h Wed Aug 27 12:45:11 2008 +0100
+++ b/src/qemu_conf.h Wed Aug 27 14:09:26 2008 +0100
@@ -49,7 +49,7 @@
/* Main driver state */
struct qemud_driver {
- int qemuVersion;
+ unsigned int qemuVersion;
int nextvmid;
virDomainObjPtr domains;
@@ -86,13 +86,13 @@
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 -r c3d345142e1b src/qemu_driver.c
--- a/src/qemu_driver.c Wed Aug 27 12:45:11 2008 +0100
+++ b/src/qemu_driver.c Wed Aug 27 14:09:26 2008 +0100
@@ -312,6 +312,7 @@
qemudActive(void) {
virDomainObjPtr dom = qemu_driver->domains;
virNetworkObjPtr net = qemu_driver->networks;
+
while (dom) {
if (virDomainIsActive(dom))
return 1;
@@ -846,7 +847,7 @@
struct stat sb;
int *tapfds = NULL;
int ntapfds = 0;
- int qemuCmdFlags;
+ unsigned int qemuCmdFlags;
fd_set keepfd;
FD_ZERO(&keepfd);
@@ -1509,19 +1510,11 @@
}
- 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 -r c3d345142e1b src/remote_internal.c
--- a/src/remote_internal.c Wed Aug 27 12:45:11 2008 +0100
+++ b/src/remote_internal.c Wed Aug 27 14:09:26 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,62 +222,17 @@
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 @@
goto retry_wait;
}
- return (0);
+ return 0;
}
#endif
@@ -349,7 +305,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;
@@ -693,40 +649,9 @@
goto failed;
}
- pid = fork ();
- if (pid == -1) {
- errorf (conn, VIR_ERR_SYSTEM_ERROR,
- _("unable to fork external network transport: %s"),
- 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");
- _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);
- }
+ if (virExec(conn, (const char**)cmd_argv, NULL, NULL,
+ &pid, sv[1], &(sv[1]), NULL, VIR_EXEC_NONE) < 0)
+ goto failed;
/* Parent continues here. */
close (sv[1]);
--
|: 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 :|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list