On Fri, Oct 29, 2010 at 04:43:19PM +0800, Osier Yang wrote: > Currently only record timestamps for domain start and shutdown, for > domain start, record timestamps before qemu command line, for domain > shutdown, just says it's shutting down. > > * src/qemu/qemu_driver.c (qemudStartVMDaemon, qemudShutdownVMDaemon) > --- > src/qemu/qemu_driver.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++- > 1 files changed, 60 insertions(+), 2 deletions(-) > > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index 1eea3a9..89b4d11 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -3828,6 +3828,10 @@ static int qemudStartVMDaemon(virConnectPtr conn, > char ebuf[1024]; > char *pidfile = NULL; > int logfile = -1; > + struct timeval cur_time; > + struct tm time_info; > + char timestr[100]; > + char *timestamp; > qemuDomainObjPrivatePtr priv = vm->privateData; > > struct qemudHookData hookData; > @@ -4015,7 +4019,27 @@ static int qemudStartVMDaemon(virConnectPtr conn, > goto cleanup; > } > > + gettimeofday(&cur_time, NULL); > + localtime_r(&cur_time.tv_sec,&time_info); > + > + strftime(timestr, sizeof(timestr), "%Y-%m-%d %H:%M:%S",&time_info); > + > + if (virAsprintf(&timestamp, "%s.%3d: ", > + timestr, (int) cur_time.tv_usec / 1000)< 0) { > + VIR_FREE(timestamp); > + virReportOOMError(); > + goto cleanup; > + } This is a rather unwieldly block of code, that is just dieing to have a helper function in src/util, so we don't need to duplicate it everywhere

Regards, Daniel

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.

> @@ -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.

> + } 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.

> + > + 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.

> @@ -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)); > }

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 | 49 ++++++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 47 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0612e69..1b03ef1 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -81,6 +81,7 @@ #include "hooks.h" #include "storage_file.h" #include "virtaudit.h" +#include "files.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -3866,6 +3867,7 @@ static int qemudStartVMDaemon(virConnectPtr conn, char ebuf[1024]; char *pidfile = NULL; int logfile = -1; + char *timestamp; qemuDomainObjPrivatePtr priv = vm->privateData; struct qemudHookData hookData; @@ -4053,7 +4055,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); + } + tmp = progenv; + while (*tmp) { if (safewrite(logfile, *tmp, strlen(*tmp))< 0) VIR_WARN("Unable to write envv to logfile: %s", @@ -4204,7 +4216,6 @@ cleanup: return -1; } - static void qemudShutdownVMDaemon(struct qemud_driver *driver, virDomainObjPtr vm, int migrated) { @@ -4214,10 +4225,45 @@ static void qemudShutdownVMDaemon(struct qemud_driver *driver, virErrorPtr orig_err; virDomainDefPtr def; int i; + int logfile = -1; + char *timestamp; + char ebuf[1024]; VIR_DEBUG("Shutting down VM '%s' pid=%d migrated=%d", vm->def->name, vm->pid, migrated); + 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(); + } else { + char *shutdownMsg; + + if ((virAsprintf(&shutdownMsg, "%s: shutting down\n", + timestamp))< 0) { + virReportOOMError(); + } else { + if (safewrite(logfile, shutdownMsg, strlen(shutdownMsg))< 0) { + VIR_WARN("Unable to write shutdownMsg to logfile: %s", + virStrerror(errno, ebuf, sizeof ebuf)); + } + + VIR_FREE(shutdownMsg); + } + + VIR_FREE(timestamp); + } + + if (VIR_CLOSE(logfile)< 0) + VIR_WARN("Unable to close logfile: %s", + virStrerror(errno, ebuf, sizeof ebuf)); + } + /* This method is routinely used in clean up paths. Disable error * reporting so we don't squash a legit error. */ orig_err = virSaveLastError(); @@ -4353,7 +4399,6 @@ retry: } } - static virDrvOpenStatus qemudOpen(virConnectPtr conn, virConnectAuthPtr auth ATTRIBUTE_UNUSED, int flags ATTRIBUTE_UNUSED) { -- 1.7.2.3 -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list