Re: [libvirt] [PATCH] [RESEND] qemu: record timestamp in qemu domain log
于 2010年11月16日 23:18, 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
yep, right, thanks. just replied with the updated patch.
Regards,
Daniel
- Osier