Re: [PATCH v2 12/13] qemu: convert monitor to use qemuDomainLogContextPtr indirectly

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]