[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]