On 11/12/2015 12:19 PM, Daniel P. Berrange wrote:
> Currently the QEMU monitor is given an FD to the logfile. This
> won't work in the future with virtlogd, so it needs to use the
> qemuDomainLogContextPtr instead, but it shouldn't directly
> access that object either. So define a callback that the
> monitor can use for reporting errors from the log file.
>
> Signed-off-by: Daniel P. Berrange <berrange@xxxxxxxxxx>
> ---
> src/qemu/qemu_domain.c | 12 ------
> src/qemu/qemu_domain.h | 2 -
> src/qemu/qemu_migration.c | 2 +-
> src/qemu/qemu_monitor.c | 51 ++++++++++++++------------
> src/qemu/qemu_monitor.h | 8 +++-
> src/qemu/qemu_process.c | 93 ++++++++++++++++++++++++-----------------------
> src/qemu/qemu_process.h | 4 --
> 7 files changed, 84 insertions(+), 88 deletions(-)
>
[...]
> index d3f0c09..bd5d9ca 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -94,9 +94,10 @@ struct _qemuMonitor {
> char *balloonpath;
> bool ballooninit;
>
> - /* Log file fd of the qemu process to dig for usable info */
> - int logfd;
> - off_t logpos;
> + /* Log file context of the qemu process to dig for usable info */
> + qemuMonitorReportDomainLogError logFunc;
> + void *logOpaque;
> + virFreeCallback logDestroy;
> };
>
> /**
> @@ -315,7 +316,6 @@ qemuMonitorDispose(void *obj)
> VIR_FREE(mon->buffer);
> virJSONValueFree(mon->options);
> VIR_FREE(mon->balloonpath);
> - VIR_FORCE_CLOSE(mon->logfd);
> }
>
>
> @@ -706,18 +706,17 @@ qemuMonitorIO(int watch, int fd, int events, void *opaque)
> }
>
> if (error || eof) {
> - if (hangup && mon->logfd != -1) {
> + if (hangup && mon->logFunc != NULL) {
> /* Check if an error message from qemu is available and if so, use
> * it to overwrite the actual message. It's done only in early
> * startup phases or during incoming migration when the message
> * from qemu is certainly more interesting than a
> * "connection reset by peer" message.
> */
> - qemuProcessReportLogError(mon->logfd,
> - mon->logpos,
> - _("early end of file from monitor, "
> - "possible problem"));
> - VIR_FORCE_CLOSE(mon->logfd);
> + mon->logFunc(mon,
> + _("early end of file from monitor, "
> + "possible problem"),
> + mon->logOpaque);
Mostly a consistency thing and not that it should happen since
qemuConnectMonitor checks for logCtxt before calling
qemuMonitorSetDomainLog; however, qemuMonitorClose and
qemuMonitorSetDomainLog check for mon->logOpaque before calling
logDestroy. Likewise, if logFunc is called with NULL (logOpaque) life
won't be good.
I don't think there's anything wrong with the current code - just caused
me to double check things.
> virCopyLastError(&mon->lastError);
> virResetLastError();
> }
> @@ -802,7 +801,6 @@ qemuMonitorOpenInternal(virDomainObjPtr vm,
> if (!(mon = virObjectLockableNew(qemuMonitorClass)))
> return NULL;
>
> - mon->logfd = -1;
> if (virCondInit(&mon->notify) < 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("cannot initialize monitor condition"));
> @@ -932,6 +930,13 @@ qemuMonitorClose(qemuMonitorPtr mon)
> VIR_FORCE_CLOSE(mon->fd);
> }
>
> + if (mon->logDestroy && mon->logOpaque) {
> + mon->logDestroy(mon->logOpaque);
> + mon->logOpaque = NULL;
> + mon->logDestroy = NULL;
> + mon->logFunc = NULL;
> + }
> +
> /* In case another thread is waiting for its monitor command to be
> * processed, we need to wake it up with appropriate error set.
> */
> @@ -3646,21 +3651,21 @@ qemuMonitorGetDeviceAliases(qemuMonitorPtr mon,
> * early startup errors of qemu.
> *
> * @mon: Monitor object to set the log file reading on
> - * @logfd: File descriptor of the already open log file
> - * @pos: position to read errors from
> + * @func: the callback to report errors
> + * @opaque: data to pass to @func
Missing @destroy
> */
> -int
> -qemuMonitorSetDomainLog(qemuMonitorPtr mon, int logfd, off_t pos)
> +void
> +qemuMonitorSetDomainLog(qemuMonitorPtr mon,
> + qemuMonitorReportDomainLogError func,
> + void *opaque,
> + virFreeCallback destroy)
[...]
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 132b3eb..4a2e2c1 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -1544,9 +1544,22 @@ static qemuMonitorCallbacks monitorCallbacks = {
> .domainMigrationStatus = qemuProcessHandleMigrationStatus,
> };
>
> +static void
> +qemuProcessMonitorReportLogError(qemuMonitorPtr mon,
> + const char *msg,
> + void *opaque);
> +
> +
> +static void
> +qemuProcessMonitorLogFree(void *opaque)
> +{
> + qemuDomainLogContextPtr logCtxt = opaque;
This could check opaque != NULL and avoid the other checks for
logDestroy && logOpaque - your call.
> + qemuDomainLogContextFree(logCtxt);
> +}
> +
> static int
> qemuConnectMonitor(virQEMUDriverPtr driver, virDomainObjPtr vm, int asyncJob,
> - int logfd, off_t pos)
> + qemuDomainLogContextPtr logCtxt)
> {
> qemuDomainObjPrivatePtr priv = vm->privateData;
> int ret = -1;
> @@ -1572,8 +1585,13 @@ qemuConnectMonitor(virQEMUDriverPtr driver, virDomainObjPtr vm, int asyncJob,
> &monitorCallbacks,
> driver);
>
> - if (mon && logfd != -1 && pos != -1)
> - ignore_value(qemuMonitorSetDomainLog(mon, logfd, pos));
> + if (mon && logCtxt) {
> + qemuDomainLogContextRef(logCtxt);
> + qemuMonitorSetDomainLog(mon,
> + qemuProcessMonitorReportLogError,
> + logCtxt,
> + qemuProcessMonitorLogFree);
> + }
>
> virObjectLock(vm);
> virObjectUnref(vm);
> @@ -1626,36 +1644,22 @@ qemuConnectMonitor(virQEMUDriverPtr driver, virDomainObjPtr vm, int asyncJob,
>
> /**
> * qemuProcessReadLog: Read log file of a qemu VM
> - * @fd: File descriptor of the log file
> + * @logCtxt: the domain log context
> * @msg: pointer to buffer to store the read messages in
> - * @off: Offset to start reading from
> *
> * Reads log of a qemu VM. Skips messages not produced by qemu or irrelevant
> * messages. Returns returns 0 on success or -1 on error
> */
> static int
> -qemuProcessReadLog(int fd, off_t offset, char **msg)
> +qemuProcessReadLog(qemuDomainLogContextPtr logCtxt, char **msg)
> {
> char *buf;
> - size_t buflen = 1024 * 128;
> ssize_t got;
> char *eol;
> char *filter_next;
>
> - /* Best effort jump to start of messages */
> - ignore_value(lseek(fd, offset, SEEK_SET));
> -
> - if (VIR_ALLOC_N(buf, buflen) < 0)
> - return -1;
> -
> - got = saferead(fd, buf, buflen - 1);
> - if (got < 0) {
> - virReportSystemError(errno, "%s",
> - _("Unable to read from log file"));
> + if ((got = qemuDomainLogContextRead(logCtxt, &buf)) < 0)
> return -1;
> - }
> -
> - buf[got] = '\0';
>
> /* Filter out debug messages from intermediate libvirt process */
> filter_next = buf;
> @@ -1672,24 +1676,24 @@ qemuProcessReadLog(int fd, off_t offset, char **msg)
> }
> }
>
> - if (buf[got - 1] == '\n') {
> + if (got > 0 &&
> + buf[got - 1] == '\n') {
> buf[got - 1] = '\0';
> got--;
> }
> - VIR_SHRINK_N(buf, buflen, buflen - got - 1);
> + ignore_value(VIR_REALLOC_N_QUIET(buf, got + 1));
> *msg = buf;
This is where I first saw Coverity erroneously complaining filter_next
is leaked... If I set filter_buf = NULL after the while loop there's no
complaint. Very strange.
> return 0;
> }
>
>
ACK - as long as @destroy is noted and your call on other comments.
John
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list