11:40 a.m.
"Daniel P. Berrange" <berrange(a)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.