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

[Jim Meyering]

"Daniel P. Berrange" <berrange at redhat.com> 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!

> diff -r 2591ebc40bd7 src/bridge.c
...
> @@ -680,7 +645,10 @@
>
>      argv[n++] = NULL;
>
> -    retval = brctlSpawn(argv);
> +    if (virRun(NULL, argv, NULL) < 0)
> +        retval = errno;

Using errno here isn't always kosher, since _virExec doesn't always
preserve it when things go wrong (the ReportError call and close
calls after "cleanup:" can clobber errno).
But one can argue that it doesn't matter too much, since virExec
does diagnose each failure.  However, that would suggest not
using errno upon virRun failure.

...
> -    retval = brctlSpawn(argv);
> +    if (virRun(NULL, argv, NULL) < 0)
> +        retval = errno;

Likewise.

...
> 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 };

If you have to use just one environment variable, use LC_ALL=C.
It trumps all others, when it comes to locale-related things.

>      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 you make the above 4 variables unsigned
and adjust the %d formats accordingly, the version
parsing will be a little tighter.

>      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;
> +    }

Any reason not to use saferead in place of this while-loop?

> +    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",

If version can really be NULL, as the above "if (version)" tests
suggest, then you'll want to change "*version" here, to e.g.,
version ? *version : "NA"

Same for *flags.

> +               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);

You've probably already fixed this, but "status" here is a
combination of exit status and signal number.

> +        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);
>      }
...
>  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";

Save an arg slot and use "--timeout=30"?
That's slightly more readable, too.

> +    daemonargs[3] = NULL;
>
...
> +    if (virExec(NULL, daemonargs, NULL, &pid, -1, NULL, NULL, VIR_EXEC_DAEMON) < 0)
> +        error(conn, VIR_ERR_INTERNAL_ERROR, _("failed to fork libvirtd binary"));

s/ binary//, in case someday it's not a binary.

...
> +        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));

Similar to the issue with virRun, whenever virExec returns nonzero,
it has already diagnosed the error and errno may be clobbered.