[libvirt] [PATCH] qemu: Record timestamp for qemu domain log

[Osier]

On 11/02/2010 06:59 AM, Eric Blake wrote:
> On 11/01/2010 06:46 AM, Osier Yang wrote:
>> Currently only support domain start and domain shutdown, for domain
>> start, adding timestamp before qemu command line, for domain shutdown,
>> just say it's shutting down with timestamp.
>>
>> * src/qemu/qemu_driver.c
>> ---
>>   src/qemu/qemu_driver.c |   42 +++++++++++++++++++++++++++++++++++++++++-
>>   1 files changed, 41 insertions(+), 1 deletions(-)
>>
>> +    } else if (safewrite(logfile, timestamp, strlen(timestamp))<  0) {
>> +        VIR_FREE(timestamp);
>> +        VIR_WARN("Unable to write timestamp to logfile: %s",
>> +                 virStrerror(errno, ebuf, sizeof ebuf));
>
> VIR_FREE might clobber errno.  Swap these two lines.

ok, will update

>
>> @@ -4178,6 +4188,31 @@ static void qemudShutdownVMDaemon(struct qemud_driver *driver,
>>       virErrorPtr orig_err;
>>       virDomainDefPtr def;
>>       int i;
>> +    int logfile = -1;
>> +    char *timestamp;
>> +    char ebuf[1024];
>> +
>> +    VIR_DEBUG0("Creating domain log file");
>> +    if ((logfile = qemudLogFD(driver, vm->def->name))<  0) {
>> +        /* To not break the normal domain shutdown process, skip the
>> +         * timestamp log writing if failed on opening log file. */
>> +        VIR_WARN("Unable to open logfile: %s",
>> +                  virStrerror(errno, ebuf, sizeof ebuf));
>> +    } else {
>> +        if ((timestamp = virTimestamp()) == NULL) {
>> +            virReportOOMError();
>> +            goto cleanup;
>
> Hmm.  The rest of this function is best-effort cleanup, since there is
> no return value.  If you run out of memory while trying to cleanup, you
> should STILL try and clean up the rest of the resources freed later in
> this function, rather than immediately jumping to your cleanup: handler.

well, actually I just need to VIR_CLOSE the fd, so will not use goto 
statement in this funtion anymore. but VIR_CLOSE the fd here instead. :-)

>
>> +        } else {
>> +            strcat(timestamp, "shutting down\n");
>
> Ouch. Buffer overflow - you can't guarantee whether timestamp was
> overallocated with enough memory for you to blindly strcat() data onto
> the end of it.

hum, will update.

>
>> +
>> +            if (safewrite(logfile, timestamp, strlen(timestamp))<  0) {
>> +                VIR_WARN("Unable to write timestamp to logfile: %s",
>> +                          virStrerror(errno, ebuf, sizeof ebuf));
>> +            }
>> +
>> +            VIR_FREE(timestamp);
>> +        }
>> +    }
>>
>>       VIR_DEBUG("Shutting down VM '%s' pid=%d migrated=%d",
>>                 vm->def->name, vm->pid, migrated);
>
> Also, I think you want your attempts to write to a log file to occur
> after this VIR_DEBUG statement.

yep, after this will be better.

>
>> @@ -4315,6 +4350,11 @@ retry:
>>           virSetError(orig_err);
>>           virFreeError(orig_err);
>>       }
>> +
>> +cleanup:
>> +    if (VIR_CLOSE(logfile)<  0)
>> +        VIR_WARN("Unable to close logfile: %s",
>> +                 virStrerror(errno, ebuf, sizeof ebuf));
>>   }
>