Hi Dan,
On Wed, 2007-02-21 at 15:45 +0000, Daniel P. Berrange wrote:
> At the same time I also check for whether -no-kqemu is available
> so we can turn off KQEMU support if it is not available.
Score! :)
> if (vm->def->graphicsType == QEMUD_GRAPHICS_VNC) {
> char port[10];
> - snprintf(port, 10, "%d", vm->def->vncActivePort - 5900);
> + int ret;
> + ret = snprintf(port, sizeof(port),
> + (server->qemuVersion >= 9000 ?
> + ":%d" : "%d"),
> + vm->def->vncActivePort - 5900);
How about something like:
server->qemuCmdFlags & QEMUD_CMD_VNC_PORT_NEEDS_COLON ? ":%d" : "%d"
That way we could keep the actual logic around what version does what
in qemudExtractVersion()
(Ditto for QEMUD_CMD_HAS_KQEMU ...)
> +int qemudExtractVersion(const char *qemu, int *version, int *hasKQEMU) {
> + int child;
> + int newstdout[2];
> +
> + if (pipe(newstdout) < 0) {
> + return -1;
> + }
> +
> + if ((child = fork()) < 0) {
Close the pipes here
> + if ((got = read(newstdout[0], help, sizeof(help)-1)) < 0)
> + goto cleanup2;
Handle EINTR here ... it's quite a likely place to hit it, I guess
(e.g. SIGCHLD?)
> + qemudDebug("Version %d %d %d Cooked version: %d, with KQEMU ? %d",
Need a "\n"
> + major, minor, micro, *version, *hasKQEMU);
> +
> + cleanup2:
> + if (close(newstdout[0]) < 0)
> + ret = -1;
> +
> + if (waitpid(child, &got, 0) != child ||
> + WEXITSTATUS(got) != 1) {
> + qemudLog(QEMUD_ERR, "Unexpected exit status from qemu %d pid %d", got, child);
> + ret = -1;
> + }
Don't know that we actually want to fail under both these
circumstances.
> Index: qemud/driver.h
> ===================================================================
> RCS file: /data/cvs/libvirt/qemud/driver.h,v
> retrieving revision 1.3
> diff -u -p -r1.3 driver.h
> --- qemud/driver.h 14 Feb 2007 16:20:38 -0000 1.3
> +++ qemud/driver.h 21 Feb 2007 15:40:41 -0000
> @@ -30,6 +30,7 @@
> void qemudReportError(struct qemud_server *server,
> int code, const char *fmt, ...);
>
> +int qemudExtractVersion(const char *qemu, int *version, int *hasKQEMU);
The driver seems to be a strange place to put this function.
> Index: qemud/qemud.c
> ===================================================================
> RCS file: /data/cvs/libvirt/qemud/qemud.c,v
> retrieving revision 1.23
> diff -u -p -r1.23 qemud.c
> --- qemud/qemud.c 20 Feb 2007 17:51:41 -0000 1.23
> +++ qemud/qemud.c 21 Feb 2007 15:40:42 -0000
> @@ -411,20 +411,26 @@ static struct qemud_server *qemudInitial
> + if (!(binary = qemudLocateBinaryForArch(server, QEMUD_VIRT_QEMU, "i686")))
> + goto cleanup;
> + if (qemudExtractVersion(binary, &server->qemuVersion, &server->qemuHasKQEMU) < 0)
> + goto cleanup;
> + free(binary);
> + binary = NULL;
Note, this means we don't have support for networks if something's
wrong with qemu ... think we should just try and locate the binary when
starting domains and return an error if it fails.
Cheers,
Mark.