Switch over fork/exec code to use virExec

ChangeLog

@@ -1,3 +1,9 @@
+Fri Aug 29 08:04:15 BST 2008 Daniel P. Berrange <berrange@redhat.com>
+
+	* 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 <berrange@redhat.com>
 
 	* src/util.c: Fix off-by-one to allow making of paths at root

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];
-
+    const char *const progargv[] = {
-    n = 1 + /* brctl */
+        BRCTL, "setfd", bridge, delayStr, NULL
-        1 + /* setfd */
+    };
-        1 + /* brige name */
-        1; /* value */
 
     snprintf(delayStr, sizeof(delayStr), "%d", delay);
 
-    if (VIR_ALLOC_N(argv, n + 1) < 0)
+    if (virRun(NULL, progargv, NULL) < 0)
-        goto error;
+        return -1;
 
-    n = 0;
+    return 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;
 }
 
 /**
@@ -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;
+    const char *setting = enable ? "on" : "off";
-    int retval = ENOMEM;
+    const char *const progargv[] = {
-    int n;
+        BRCTL, "stp", bridge, setting, NULL
+    };
 
-    n = 1 + /* brctl */
+    if (virRun(NULL, progargv, NULL) < 0)
-        1 + /* stp */
+        return -1;
-        1 + /* brige name */
-        1;  /* value */
 
-    if (VIR_ALLOC_N(argv, n + 1) < 0)
+    return 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;
 }
 
 #endif /* WITH_QEMU || WITH_LXC */

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 */
+    proxyarg[0] = proxyPath;
-    pid = fork();
+    proxyarg[1] = NULL;
-    if (pid == 0) {
-        long open_max;
-        long i;
 
-        /* don't hold open fd opened from the client of the library */
+    if (virExec(NULL, proxyarg, NULL, NULL,
-        open_max = sysconf (_SC_OPEN_MAX);
+                &pid, -1, NULL, NULL, VIR_EXEC_DAEMON) < 0)
-        for (i = 0; i < open_max; i++)
+        fprintf(stderr, "Failed to fork libvirt_proxy\n");
-            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);
-    }
 
     /*
      * do a waitpid on the intermediate process to avoid zombies.

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)
+    if (retflags)
-        *flags = 0;
+        *retflags = 0;
-    if (version)
+    if (retversion)
-        *version = 0;
+        *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 */
+    while (got < (sizeof(help)-1)) {
-        /* Just in case QEMU is translated someday we force to C locale.. */
+        int len;
-        const char *const qemuenv[] = { "LANG=C", NULL };
+        if ((len = saferead(newstdout, help+got, sizeof(help)-got-1)) < 0)
-
-        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;
-
+        if (!len)
-        while (got < (sizeof(help)-1)) {
+            break;
-            int len;
+        got += 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;
     }
+    help[got] = '\0';
+
+    if (sscanf(help, "QEMU PC emulator version %u.%u.%u",
+               &major, &minor, &micro) != 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,

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,
+                                         unsigned int *version,
-                                         int *flags);
+                                         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,

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))) {
+    if (brSetForwardDelay(driver->brctl, network->def->bridge, network->def->delay) < 0)
-        qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
-                         _("failed to set bridge forward delay to %ld"),
-                         network->def->delay);
         goto err_delbr;
-    }
 
-    if ((err = brSetEnableSTP(driver->brctl, network->def->bridge, network->def->stp ? 1 : 0))) {
+    if (brSetEnableSTP(driver->brctl, network->def->bridge, network->def->stp ? 1 : 0) < 0)
-        qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
-                         _("failed to set bridge STP to %s"),
-                         network->def->stp ? "on" : "off");
         goto err_delbr;
-    }
 
     if (network->def->ipAddress &&
         (err = brSetInetAddress(driver->brctl, network->def->bridge, network->def->ipAddress))) {

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);
+        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, 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 (virExec(conn, (const char**)cmd_argv, NULL, NULL,
-        if (pid == -1) {
+                    &pid, sv[1], &(sv[1]), NULL, VIR_EXEC_NONE) < 0)
-            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);
-        }
 
         /* Parent continues here. */
         close (sv[1]);