[libvirt] [PATCH] qemu: Record timestamp for qemu domain log
Osier
jyang at redhat.com
Tue Nov 2 05:37:41 UTC 2010
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));
>> }
>
More information about the libvir-list
mailing list