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@xxxxxxxxxx>
> ---
> 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;
[...]
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list