"Daniel P. Berrange" <berrange@xxxxxxxxxx> 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.
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list