[libvirt] [PATCH] qemu driver: use lseek64 for setting position in logfile
While doing some testing with Qemu and creating huge logfiles I encountered the case where the VM could not start anymore due to the lseek() to the end of the Qemu VM's log file failing. The patch below replaces the two occurrences of lseek() in the relevant path with lseek64() and solves this problem. It may be a good idea to look at other occurrences of lseek() as well whether they should be replaced. off_t is 8 bytes long (64 bit), so it doesn't need to be replaced with the explicit off64_t. To reproduce this error, you could do the following: dd if=/dev/zero of=/var/log/libvirt/qemu/<name of VM>.log bs=1024 count=$((1024*2048)) and you should get an error like this: error: Failed to start domain <name of VM> error: Unable to seek to -2147482651 in /var/log/libvirt/qemu/<name of VM>.log: Success Signed-off-by: Stefan Berger <stefanb@us.ibm.com> --- src/qemu/qemu_driver.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) Index: libvirt-acl/src/qemu/qemu_driver.c =================================================================== --- libvirt-acl.orig/src/qemu/qemu_driver.c +++ libvirt-acl/src/qemu/qemu_driver.c @@ -238,7 +238,7 @@ qemudLogReadFD(const char* logDir, const VIR_FREE(logfile); return -1; } - if (pos < 0 || lseek(fd, pos, SEEK_SET) < 0) { + if (pos < 0 || lseek64(fd, pos, SEEK_SET) < 0) { virReportSystemError(pos < 0 ? 0 : errno, _("Unable to seek to %lld in %s"), (long long) pos, logfile); @@ -2624,7 +2624,7 @@ static int qemudStartVMDaemon(virConnect enum virVMOperationType vmop) { int ret; unsigned long long qemuCmdFlags; - int pos = -1; + off_t pos = -1; char ebuf[1024]; char *pidfile = NULL; int logfile = -1; @@ -2839,7 +2839,7 @@ static int qemudStartVMDaemon(virConnect virCommandWriteArgLog(cmd, logfile); - if ((pos = lseek(logfile, 0, SEEK_END)) < 0) + if ((pos = lseek64(logfile, 0, SEEK_END)) < 0) VIR_WARN("Unable to seek to end of logfile: %s", virStrerror(errno, ebuf, sizeof ebuf));
libvir-list-bounces@redhat.com wrote on 01/04/2011 09:46:07 AM:
While doing some testing with Qemu and creating huge logfiles I encountered the case where the VM could not start anymore due to the lseek() to the end of the Qemu VM's log file failing. The patch below replaces the two occurrences of lseek() in the relevant path with lseek64() and solves this problem. It may be a good idea to look at other occurrences of lseek() as well whether they should be replaced. off_t is 8 bytes long (64 bit), so it doesn't need to be replaced with the explicit off64_t.
To reproduce this error, you could do the following:
dd if=/dev/zero of=/var/log/libvirt/qemu/<name of VM>.log bs=1024 count=$((1024*2048))
and you should get an error like this:
error: Failed to start domain <name of VM> error: Unable to seek to -2147482651 in /var/log/libvirt/qemu/<name of VM>.log: Success
Signed-off-by: Stefan Berger <stefanb@us.ibm.com>
--- src/qemu/qemu_driver.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
Index: libvirt-acl/src/qemu/qemu_driver.c =================================================================== --- libvirt-acl.orig/src/qemu/qemu_driver.c +++ libvirt-acl/src/qemu/qemu_driver.c
[...]
@@ -2624,7 +2624,7 @@ static int qemudStartVMDaemon(virConnect enum virVMOperationType vmop) { int ret; unsigned long long qemuCmdFlags; - int pos = -1; + off_t pos = -1; char ebuf[1024]; char *pidfile = NULL; int logfile = -1;
... actually this is really the only hunk that's necessary to fix this problem. Stefan
[hmm, are you aware that messages from you are adding a cc to libvir-list-bounces?] On 01/04/2011 08:02 AM, Stefan Berger wrote:
While doing some testing with Qemu and creating huge logfiles I encountered the case where the VM could not start anymore due to the lseek() to the end of the Qemu VM's log file failing. The patch below replaces the two occurrences of lseek() in the relevant path with lseek64() and solves this problem. It may be a good idea to look at other occurrences of lseek() as well whether they should be replaced. off_t is 8 bytes long (64 bit), so it doesn't need to be replaced with the explicit off64_t.
NACK to the bulk of this patch. Gnulib already guarantees that we have large-file support (and therefore, off_t should already be off64_t on platforms that support dual mode off_t sizing); we should NOT be explicitly referencing the non-standard off64_t or lseek64, since they do not exist on all platforms.
@@ -2624,7 +2624,7 @@ static int qemudStartVMDaemon(virConnect enum virVMOperationType vmop) { int ret; unsigned long long qemuCmdFlags; - int pos = -1; + off_t pos = -1; char ebuf[1024]; char *pidfile = NULL; int logfile = -1;
... actually this is really the only hunk that's necessary to fix this problem.
Agree that changing that one mistaken type should fix things. Can you resubmit as a v2 with a fixed commit message, at which point I will feel more comfortable giving ack? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Tue, Jan 04, 2011 at 09:46:07AM -0500, Stefan Berger wrote:
While doing some testing with Qemu and creating huge logfiles I encountered the case where the VM could not start anymore due to the lseek() to the end of the Qemu VM's log file failing. The patch below replaces the two occurrences of lseek() in the relevant path with lseek64() and solves this problem. It may be a good idea to look at other occurrences of lseek() as well whether they should be replaced. off_t is 8 bytes long (64 bit), so it doesn't need to be replaced with the explicit off64_t.
To reproduce this error, you could do the following:
dd if=/dev/zero of=/var/log/libvirt/qemu/<name of VM>.log bs=1024 count=$((1024*2048))
and you should get an error like this:
error: Failed to start domain <name of VM> error: Unable to seek to -2147482651 in /var/log/libvirt/qemu/<name of VM>.log: Success
Signed-off-by: Stefan Berger <stefanb@us.ibm.com>
--- src/qemu/qemu_driver.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
Index: libvirt-acl/src/qemu/qemu_driver.c =================================================================== --- libvirt-acl.orig/src/qemu/qemu_driver.c +++ libvirt-acl/src/qemu/qemu_driver.c @@ -238,7 +238,7 @@ qemudLogReadFD(const char* logDir, const VIR_FREE(logfile); return -1; } - if (pos < 0 || lseek(fd, pos, SEEK_SET) < 0) { + if (pos < 0 || lseek64(fd, pos, SEEK_SET) < 0) { virReportSystemError(pos < 0 ? 0 : errno, _("Unable to seek to %lld in %s"), (long long) pos, logfile); @@ -2624,7 +2624,7 @@ static int qemudStartVMDaemon(virConnect enum virVMOperationType vmop) { int ret; unsigned long long qemuCmdFlags; - int pos = -1; + off_t pos = -1; char ebuf[1024]; char *pidfile = NULL; int logfile = -1; @@ -2839,7 +2839,7 @@ static int qemudStartVMDaemon(virConnect
virCommandWriteArgLog(cmd, logfile);
- if ((pos = lseek(logfile, 0, SEEK_END)) < 0) + if ((pos = lseek64(logfile, 0, SEEK_END)) < 0) VIR_WARN("Unable to seek to end of logfile: %s", virStrerror(errno, ebuf, sizeof ebuf));
Looks fine to me, ACK But the real problem is getting huge log files, there is a trade off between the default log level and the amount of information we want to have handy in case something goes wrong. One of the thing I plan to implement is to change the default behaviour to log all message to an in-memory ring buffer, and in case of an error at runtime log that buffer collecting the past recent detailed informations of what happened before the current error. I assume this will have to be tuned too to avoid excessive default logging, but this should improve at least the usefulness of those logs, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On 01/04/2011 07:38 PM, Daniel Veillard wrote:
On Tue, Jan 04, 2011 at 09:46:07AM -0500, Stefan Berger wrote: Looks fine to me, ACK
I checked it in earlier today with the lseek64's removed...
But the real problem is getting huge log files, there is a trade off between the default log level and the amount of information we want to have handy in case something goes wrong. This was me activating a DEBUG-#define in a Qemu driver and doing suspend/resume tests for a couple of hours. One of the thing I plan to implement is to change the default behaviour to log all message to an in-memory ring buffer, and in case of an error at runtime log that buffer collecting the past recent detailed informations of what happened before the current error. I assume this will have to be tuned too to avoid excessive default logging, but this should improve at least the usefulness of those logs, I guess you would need to find some sort of a trigger for the 'buffer
collecting'. The tests I ran just redirected everything fprintf'ed to stdout/stderr by Qemu into that log file, which was good for this purpose. Typically Qemu is pretty quiet.
Stefan
Daniel
participants (4)
-
Daniel Veillard -
Eric Blake -
Stefan Berger -
Stefan Berger