[libvirt] [PATCH] qemu: record timestamps in qemu guest log
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(×tamp, "%s.%3d: ", + timestr, (int) cur_time.tv_usec / 1000) < 0) { + VIR_FREE(timestamp); + virReportOOMError(); + goto cleanup; + } + + 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", @@ -4166,7 +4190,6 @@ cleanup: return -1; } - static void qemudShutdownVMDaemon(struct qemud_driver *driver, virDomainObjPtr vm, int migrated) { @@ -4176,6 +4199,42 @@ static void qemudShutdownVMDaemon(struct qemud_driver *driver, virErrorPtr orig_err; virDomainDefPtr def; int i; + int logfile = -1; + char timestr[100]; + char *timestamp; + char ebuf[1024]; + struct timeval cur_time; + struct tm time_info; + + 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 { + 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(×tamp, "%s.%3d: shutting down\n", + timestr, (int) cur_time.tv_usec / 1000) < 0) { + VIR_FREE(timestamp); + virReportOOMError(); + } + + if (close(logfile) < 0) + VIR_WARN("Unable to close logfile: %s", + virStrerror(errno, ebuf, sizeof ebuf)); + } + + 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); @@ -4315,7 +4374,6 @@ retry: } } - static virDrvOpenStatus qemudOpen(virConnectPtr conn, virConnectAuthPtr auth ATTRIBUTE_UNUSED, int flags ATTRIBUTE_UNUSED) { -- 1.7.2.3
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(×tamp, "%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 -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On 10/29/2010 05:25 PM, Daniel P. Berrange wrote:
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(×tamp, "%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
Agree, yes, indeed see similar codes used in some other places, hesitated about if it will be good to replace them, will write a helper function, and update the patch then.. Thanks. - Osier
Regards, Daniel
* src/util/util.h * src/util/util.c * src/libvirt_private.syms --- src/libvirt_private.syms | 1 + src/util/util.c | 20 ++++++++++++++++++++ src/util/util.h | 3 +++ 3 files changed, 24 insertions(+), 0 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index cf64bd3..bd77a34 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -779,6 +779,7 @@ virStrToLong_ui; virStrToLong_ull; virStrcpy; virStrncpy; +virTimestamp; # uuid.h diff --git a/src/util/util.c b/src/util/util.c index 2632fe7..6f83730 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -38,6 +38,7 @@ #include <sys/stat.h> #include <sys/ioctl.h> #include <sys/wait.h> +#include <sys/time.h> #if HAVE_MMAP # include <sys/mman.h> #endif @@ -2912,3 +2913,22 @@ int virBuildPathInternal(char **path, ...) return ret; } + +char *virTimestamp(void) { + struct timeval cur_time; + struct tm time_info; + char timestr[100]; + char *timestamp; + + 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(×tamp, "%s.%3d: ", + timestr, (int) cur_time.tv_usec / 1000) < 0) { + return NULL; + } + + return timestamp; +} diff --git a/src/util/util.h b/src/util/util.h index 5de4fd6..a240d87 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -160,6 +160,7 @@ int virFileOpenTtyAt(const char *ptmx, char* virFilePid(const char *dir, const char *name); + int virFileWritePidPath(const char *path, pid_t pid) ATTRIBUTE_RETURN_CHECK; int virFileWritePid(const char *dir, @@ -277,4 +278,6 @@ void virFileWaitForDevices(void); # define virBuildPath(path, ...) virBuildPathInternal(path, __VA_ARGS__, NULL) int virBuildPathInternal(char **path, ...) ATTRIBUTE_SENTINEL; +char *virTimestamp(void); + #endif /* __VIR_UTIL_H__ */ -- 1.7.2.3
On 10/31/2010 10:17 PM, Osier Yang wrote:
* src/util/util.h * src/util/util.c * src/libvirt_private.syms --- src/libvirt_private.syms | 1 + src/util/util.c | 20 ++++++++++++++++++++ src/util/util.h | 3 +++ 3 files changed, 24 insertions(+), 0 deletions(-)
+ +char *virTimestamp(void) {
We typically start functions with the { on the first column of a new line.
+ if (virAsprintf(×tamp, "%s.%3d: ", + timestr, (int) cur_time.tv_usec / 1000) < 0) {
You want %03d, to ensure that you always print 3 digits of fractional seconds, even when < 100. ACK with those nits adjusted; I made the changes and pushed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 11/02/2010 07:10 AM, Eric Blake wrote:
On 10/31/2010 10:17 PM, Osier Yang wrote:
* src/util/util.h * src/util/util.c * src/libvirt_private.syms --- src/libvirt_private.syms | 1 + src/util/util.c | 20 ++++++++++++++++++++ src/util/util.h | 3 +++ 3 files changed, 24 insertions(+), 0 deletions(-)
+ +char *virTimestamp(void) {
We typically start functions with the { on the first column of a new line.
+ if (virAsprintf(×tamp, "%s.%3d: ", + timestr, (int) cur_time.tv_usec / 1000)< 0) {
You want %03d, to ensure that you always print 3 digits of fractional seconds, even when< 100.
ACK with those nits adjusted; I made the changes and pushed.
Thanks.. :) - Osier
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(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a7cce6a..963b70c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3830,6 +3830,7 @@ static int qemudStartVMDaemon(virConnectPtr conn, char ebuf[1024]; char *pidfile = NULL; int logfile = -1; + char *timestamp; qemuDomainObjPrivatePtr priv = vm->privateData; struct qemudHookData hookData; @@ -4017,7 +4018,17 @@ static int qemudStartVMDaemon(virConnectPtr conn, goto cleanup; } + if ((timestamp = virTimestamp()) == NULL) { + virReportOOMError(); + goto cleanup; + } 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)); + } + tmp = progenv; + while (*tmp) { if (safewrite(logfile, *tmp, strlen(*tmp)) < 0) VIR_WARN("Unable to write envv to logfile: %s", @@ -4168,7 +4179,6 @@ cleanup: return -1; } - static void qemudShutdownVMDaemon(struct qemud_driver *driver, virDomainObjPtr vm, int migrated) { @@ -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; + } else { + strcat(timestamp, "shutting down\n"); + + 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); @@ -4315,6 +4350,11 @@ retry: virSetError(orig_err); virFreeError(orig_err); } + +cleanup: + if (close(logfile) < 0) + VIR_WARN("Unable to close logfile: %s", + virStrerror(errno, ebuf, sizeof ebuf)); } -- 1.7.2.3
于 2010年11月01日 12:17, Osier Yang 写道:
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(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a7cce6a..963b70c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3830,6 +3830,7 @@ static int qemudStartVMDaemon(virConnectPtr conn, char ebuf[1024]; char *pidfile = NULL; int logfile = -1; + char *timestamp; qemuDomainObjPrivatePtr priv = vm->privateData;
struct qemudHookData hookData; @@ -4017,7 +4018,17 @@ static int qemudStartVMDaemon(virConnectPtr conn, goto cleanup; }
+ if ((timestamp = virTimestamp()) == NULL) { + virReportOOMError(); + goto cleanup; + } 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)); + } + tmp = progenv; + while (*tmp) { if (safewrite(logfile, *tmp, strlen(*tmp))< 0) VIR_WARN("Unable to write envv to logfile: %s", @@ -4168,7 +4179,6 @@ cleanup: return -1; }
- static void qemudShutdownVMDaemon(struct qemud_driver *driver, virDomainObjPtr vm, int migrated) { @@ -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; + } else { + strcat(timestamp, "shutting down\n"); + + 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); @@ -4315,6 +4350,11 @@ retry: virSetError(orig_err); virFreeError(orig_err); } + +cleanup: + if (close(logfile)< 0)
ah, I need to use VIR_CLOSE instead.. will update. - Osier
+ VIR_WARN("Unable to close logfile: %s", + virStrerror(errno, ebuf, sizeof ebuf)); }
-- 1.7.2.3
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a7cce6a..963b70c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3830,6 +3830,7 @@ static int qemudStartVMDaemon(virConnectPtr conn, char ebuf[1024]; char *pidfile = NULL; int logfile = -1; + char *timestamp; qemuDomainObjPrivatePtr priv = vm->privateData; struct qemudHookData hookData; @@ -4017,7 +4018,17 @@ static int qemudStartVMDaemon(virConnectPtr conn, goto cleanup; } + if ((timestamp = virTimestamp()) == NULL) { + virReportOOMError(); + goto cleanup; + } 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)); + } + tmp = progenv; + while (*tmp) { if (safewrite(logfile, *tmp, strlen(*tmp)) < 0) VIR_WARN("Unable to write envv to logfile: %s", @@ -4168,7 +4179,6 @@ cleanup: return -1; } - static void qemudShutdownVMDaemon(struct qemud_driver *driver, virDomainObjPtr vm, int migrated) { @@ -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; + } else { + strcat(timestamp, "shutting down\n"); + + 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); @@ -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)); } -- 1.7.2.3
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)); }
-- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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)); }
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
Could anyone help review this patch? thanks. - Osier 于 2010年11月02日 15:14, Osier Yang 写道:
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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Osier -
Osier Yang