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