"Daniel P. Berrange" <berrange@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, µ) != 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.