On 11/12/2015 12:19 PM, Daniel P. Berrange wrote:
There are two pretty similar functions qemuProcessReadLog and qemuProcessReadChildErrors. Both read from the QEMU log file and try to strip out libvirt messages. The latter than reports
s/than/then
an error, while the former lets the callers report an error.
Re-write qemuProcessReadLog so that it uses a single read into a dynamically allocated buffer. Then introduce a new qemuProcessReportLogError that calls qemuProcessReadLog and reports an error.
Convert all callers to use qemuProcessReportLogError.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/qemu/qemu_domain.c | 24 +----- src/qemu/qemu_domain.h | 2 +- src/qemu/qemu_migration.c | 2 +- src/qemu/qemu_monitor.c | 58 +++----------- src/qemu/qemu_monitor.h | 2 +- src/qemu/qemu_process.c | 200 ++++++++++++++++++---------------------------- src/qemu/qemu_process.h | 4 +- 7 files changed, 95 insertions(+), 197 deletions(-)
[...]
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 69a0f97..c09e9dc 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1549,7 +1549,7 @@ static qemuMonitorCallbacks monitorCallbacks = {
static int qemuConnectMonitor(virQEMUDriverPtr driver, virDomainObjPtr vm, int asyncJob, - int logfd) + int logfd, off_t pos) { qemuDomainObjPrivatePtr priv = vm->privateData; int ret = -1; @@ -1575,8 +1575,8 @@ qemuConnectMonitor(virQEMUDriverPtr driver, virDomainObjPtr vm, int asyncJob, &monitorCallbacks, driver);
- if (mon) - ignore_value(qemuMonitorSetDomainLog(mon, logfd)); + if (mon && logfd != -1 && pos != -1) + ignore_value(qemuMonitorSetDomainLog(mon, logfd, pos));
virObjectLock(vm); virObjectUnref(vm); @@ -1630,111 +1630,76 @@ qemuConnectMonitor(virQEMUDriverPtr driver, virDomainObjPtr vm, int asyncJob, /** * qemuProcessReadLog: Read log file of a qemu VM * @fd: File descriptor of the log file - * @buf: buffer to store the read messages - * @buflen: allocated space available in @buf + * @msg: pointer to buffer to store the read messages in
msg after off? (not that it matters that much)
* @off: Offset to start reading from - * @skipchar: Skip messages about created character devices * * Reads log of a qemu VM. Skips messages not produced by qemu or irrelevant - * messages. Returns length of the message stored in @buf, or -1 on error. + * messages. Returns returns 0 on success or -1 on error */ -int -qemuProcessReadLog(int fd, char *buf, int buflen, int off, bool skipchar) +static int +qemuProcessReadLog(int fd, off_t offset, char **msg) { - char *filter_next = buf; - ssize_t bytes; + char *buf; + size_t buflen = 1024 * 128; + ssize_t got; char *eol; + char *filter_next;
- while (off < buflen - 1) { - bytes = saferead(fd, buf + off, buflen - off - 1); - if (bytes < 0) - return -1; - - off += bytes; - buf[off] = '\0'; + /* Best effort jump to start of messages */ + ignore_value(lseek(fd, offset, SEEK_SET));
- if (bytes == 0) - break; + if (VIR_ALLOC_N(buf, buflen) < 0) + return -1;
- /* Filter out debug messages from intermediate libvirt process */ - while ((eol = strchr(filter_next, '\n'))) { - *eol = '\0'; - if (virLogProbablyLogMessage(filter_next) || - (skipchar && - STRPREFIX(filter_next, "char device redirected to"))) { - memmove(filter_next, eol + 1, off - (eol - buf)); - off -= eol + 1 - filter_next; - } else { - filter_next = eol + 1; - *eol = '\n'; - } - } + got = saferead(fd, buf, buflen - 1); + if (got < 0) { + virReportSystemError(errno, "%s", + _("Unable to read from log file"));
I know (because I ran Coverity on all the patches) that a future patch changes this, but buf is leaked here - at least for a few patches.
+ return -1; }
Additionally, theoretically 'got' could be 0 here
- return off; -} - -/* - * Read domain log and probably overwrite error if there's one in - * the domain log file. This function exists to cover the small - * window between fork() and exec() during which child may fail - * by libvirt's hand, e.g. placing onto a NUMA node failed. - */ -static int -qemuProcessReadChildErrors(virQEMUDriverPtr driver, - virDomainObjPtr vm, - off_t originalOff) -{ - int ret = -1; - int logfd; - off_t off = 0; - ssize_t bytes; - char buf[1024] = {0}; - char *eol, *filter_next = buf; - - if ((logfd = qemuDomainOpenLog(driver, vm, originalOff)) < 0) - goto cleanup; - - while (off < sizeof(buf) - 1) { - bytes = saferead(logfd, buf + off, sizeof(buf) - off - 1); - if (bytes < 0) { - VIR_WARN("unable to read from log file: %s", - virStrerror(errno, buf, sizeof(buf))); - goto cleanup; - } + buf[got] = '\0';
- off += bytes; - buf[off] = '\0'; - - if (bytes == 0) - break; - - while ((eol = strchr(filter_next, '\n'))) { - *eol = '\0'; - if (STRPREFIX(filter_next, "libvirt: ")) { - filter_next = eol + 1; - *eol = '\n'; - break; - } else { - memmove(filter_next, eol + 1, off - (eol - buf)); - off -= eol + 1 - filter_next; - } + /* Filter out debug messages from intermediate libvirt process */ + filter_next = buf; + while ((eol = strchr(filter_next, '\n'))) { + *eol = '\0'; + if (virLogProbablyLogMessage(filter_next) || + STRPREFIX(filter_next, "char device redirected to")) { + size_t skip = (eol + 1) - filter_next; + memmove(filter_next, eol + 1, (got - skip) + 1); + got -= skip; + } else { + filter_next = eol + 1; + *eol = '\n'; } }
Coverity also has a false positive here w/r/t filter_next - it claims filter_next (because it pointed to buf) is leaked when the code returns. Seems it could have something to do with when got == 1, although I don't see it. FWIW: the Coverity notes: (9) Event cond_false: Condition "eol = strchr(filter_next, 10)", taking false branch then (11) Event cond_true: Condition "buf[got - 1] == 10", taking true branch So that says, 'buf' (filter_next) didn't find '\n' in the strchr() call (e.g. taking false branch), then somehow buf[got - 1] == '\n'; If I set filter_next to NULL right after the while loop there's no error (hence my feeling on a false positive). An ACK mainly because I think what's being done is fine... John
- if (off > 0) { - /* Found an error in the log. Report it */ - virResetLastError(); - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Process exited prior to exec: %s"), - buf); + if (buf[got - 1] == '\n') { + buf[got - 1] = '\0'; + got--; } + VIR_SHRINK_N(buf, buflen, buflen - got - 1); + *msg = buf; + return 0; +}
- ret = 0;
- cleanup: - VIR_FORCE_CLOSE(logfd); - return ret;
[...]