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

[Osier Yang]

于 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