[libvirt] [PATCH] [RESEND] qemu: record timestamp in qemu domain log

[Daniel P. Berrange]

On Tue, Nov 16, 2010 at 10:54:34PM +0800, Osier Yang wrote:
> 于 2010年11月16日 21:17, Daniel P. Berrange 写道:
> >On Tue, Nov 16, 2010 at 04:11:40PM +0800, Osier Yang wrote:
> >>Currently only support domain start and shutdown, for domain start,
> >>record timestamp before the qemu command line, and for domain shutdown,
> >>just say it's shutting down with timestamp.
> >>
> >>* src/qemu/qemu_driver.c
> >>---
> >>  src/qemu/qemu_driver.c |   47 
> >>  +++++++++++++++++++++++++++++++++++++++++++++--
> >>  1 files changed, 45 insertions(+), 2 deletions(-)
> >>
> >>diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> >>index fd61864..423befb 100644
> >>--- a/src/qemu/qemu_driver.c
> >>+++ b/src/qemu/qemu_driver.c
> >>@@ -3877,6 +3877,7 @@ static int qemudStartVMDaemon(virConnectPtr conn,
> >>      char ebuf[1024];
> >>      char *pidfile = NULL;
> >>      int logfile = -1;
> >>+    char *timestamp;
> >>      qemuDomainObjPrivatePtr priv = vm->privateData;
> >>
> >>      struct qemudHookData hookData;
> >>@@ -4084,7 +4085,17 @@ static int qemudStartVMDaemon(virConnectPtr conn,
> >>              goto cleanup;
> >>      }
> >>
> >>+    if ((timestamp = virTimestamp()) == NULL) {
> >>+        virReportOOMError();
> >>+        goto cleanup;
> >>+    } else if (safewrite(logfile, timestamp, strlen(timestamp))<  0) {
> >>+        VIR_WARN("Unable to write timestamp to logfile: %s",
> >>+                 virStrerror(errno, ebuf, sizeof ebuf));
> >>+        VIR_FREE(timestamp);
> >>+    }
> >
> >Surely we want the VIR_FREE(timestamp) outside the 'else if'. This
> >code is only freeing the memory upon write failure.
> 
> In 'if'branch, "timestamp" here returned by "virTimestamp" is
> NULL, no need to free it.
> 
> And if it really needs to be freed, it should be freed
> in "virTimestamp",  "virTimestamp" invokes "virAsprintf"
> there, but "virAsprintf" guarantees *strp is NULL upon
> failure. Isn't it? just recall you reminded me before.. :-)

Consider expanding the 'else if' into a fully bracketed expression

    if ((timestamp = virTimestamp()) == NULL) {
        virReportOOMError();
        goto cleanup;
    } else {
        if (safewrite(logfile, timestamp, strlen(timestamp))<  0) {
          VIR_WARN("Unable to write timestamp to logfile: %s",
                   virStrerror(errno, ebuf, sizeof ebuf));
          VIR_FREE(timestamp);
        }
    }


The 'else' of the second 'if' statement is missing the VIR_FREE
for timestamp

Regards,
Daniel
-- 
|: Red Hat, Engineering, London    -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org        -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|