[libvirt] [PATCH] qemu: add support for error messages greater than 1024 characters
In libvirt, the default error message length is 1024 bytes. This is not enough for qemu to print long error messages such as the list of supported ARM machine models (more than 1700 chars). This is raised when the machine entry in the XML file is wrong, but there may be now or in future other verbose error messages. The above patch enables libvirt to print error messages >1024 for qemu. Signed-off-by: Michele Paolino <m.paolino@virtualopensystems.com> --- src/qemu/qemu_process.c | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index bd9546e..c2e2136 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1904,10 +1904,25 @@ cleanup: * a possible read of the fd in the monitor code doesn't influence this * error delivery option */ ignore_value(lseek(logfd, pos, SEEK_SET)); - qemuProcessReadLog(logfd, buf + len, buf_size - len - 1, 0, true); - virReportError(VIR_ERR_INTERNAL_ERROR, - _("process exited while connecting to monitor: %s"), - buf); + len = qemuProcessReadLog(logfd, buf + len, buf_size - len - 1, 0, true); + + /* virReportError error buffer is limited to 1024 byte*/ + if (len < 1024){ + virReportError(VIR_ERR_INTERNAL_ERROR, + _("process exited while connecting to monitor: %s"), + buf); + } else { + if (STRPREFIX(buf, "Supported machines are:")) + virReportError(VIR_ERR_INTERNAL_ERROR, + _("process exited while connecting to monitor:" + "please check machine model")); + else + virReportError(VIR_ERR_INTERNAL_ERROR, + _("process exited while connecting to monitor")); + + VIR_ERROR("%s", buf); + } + ret = -1; } -- 1.7.9.5
On 12/11/2013 03:33 PM, Michele Paolino wrote:
In libvirt, the default error message length is 1024 bytes. This is not enough for qemu to print long error messages such as the list of supported ARM machine models (more than 1700 chars). This is raised when the machine entry in the XML file is wrong, but there may be now or in future other verbose error messages. The above patch enables libvirt to print error messages >1024 for qemu.
Signed-off-by: Michele Paolino <m.paolino@virtualopensystems.com> --- src/qemu/qemu_process.c | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index bd9546e..c2e2136 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1904,10 +1904,25 @@ cleanup: * a possible read of the fd in the monitor code doesn't influence this * error delivery option */ ignore_value(lseek(logfd, pos, SEEK_SET)); - qemuProcessReadLog(logfd, buf + len, buf_size - len - 1, 0, true); - virReportError(VIR_ERR_INTERNAL_ERROR, - _("process exited while connecting to monitor: %s"), - buf); + len = qemuProcessReadLog(logfd, buf + len, buf_size - len - 1, 0, true); + + /* virReportError error buffer is limited to 1024 byte*/ + if (len < 1024){ + virReportError(VIR_ERR_INTERNAL_ERROR, + _("process exited while connecting to monitor: %s"), + buf); + } else { + if (STRPREFIX(buf, "Supported machines are:")) + virReportError(VIR_ERR_INTERNAL_ERROR, + _("process exited while connecting to monitor:" + "please check machine model")); + else + virReportError(VIR_ERR_INTERNAL_ERROR, + _("process exited while connecting to monitor")); + + VIR_ERROR("%s", buf); + } + ret = -1; }
This kind of error scraping is a slipper slop IMO, and for this particular case I don't think it's even that interesting. Libvirt already parses the machine types for each qemu emulator and reports it in virsh capabilities XML. Seems reasonable that we could validate the XML machine type at define time and catch an invalid machine type error long before we even try and start the VM. - Cole
On 12/14/2013 07:15 PM, Cole Robinson wrote:
On 12/11/2013 03:33 PM, Michele Paolino wrote:
In libvirt, the default error message length is 1024 bytes. This is not enough for qemu to print long error messages such as the list of supported ARM machine models (more than 1700 chars). This is raised when the machine entry in the XML file is wrong, but there may be now or in future other verbose error messages. The above patch enables libvirt to print error messages >1024 for qemu.
Signed-off-by: Michele Paolino <m.paolino@virtualopensystems.com> --- src/qemu/qemu_process.c | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index bd9546e..c2e2136 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1904,10 +1904,25 @@ cleanup: * a possible read of the fd in the monitor code doesn't influence this * error delivery option */ ignore_value(lseek(logfd, pos, SEEK_SET)); - qemuProcessReadLog(logfd, buf + len, buf_size - len - 1, 0, true); - virReportError(VIR_ERR_INTERNAL_ERROR, - _("process exited while connecting to monitor: %s"), - buf); + len = qemuProcessReadLog(logfd, buf + len, buf_size - len - 1, 0, true); + + /* virReportError error buffer is limited to 1024 byte*/ + if (len < 1024){ + virReportError(VIR_ERR_INTERNAL_ERROR, + _("process exited while connecting to monitor: %s"), + buf); + } else { + if (STRPREFIX(buf, "Supported machines are:")) + virReportError(VIR_ERR_INTERNAL_ERROR, + _("process exited while connecting to monitor:" + "please check machine model")); + else + virReportError(VIR_ERR_INTERNAL_ERROR, + _("process exited while connecting to monitor")); + + VIR_ERROR("%s", buf); + } + ret = -1; }
This kind of error scraping is a slipper slop IMO, and for this particular case I don't think it's even that interesting.
I definitely agree with that.
Libvirt already parses the machine types for each qemu emulator and reports it in virsh capabilities XML. Seems reasonable that we could validate the XML machine type at define time and catch an invalid machine type error long before we even try and start the VM.
Do we really want to refuse to define a particular guest just because the host where we're defining it doesn't currently support the given machine type? That's yet another slippery slope - for example, should we also be validating all the devices in <hostdev> at definition time? I think the proper time to do that validation is at domain start time when we should have the proper qemu binary (and necessary drivers/hardware) available.
On 12/16/13 10:27, Laine Stump wrote:
On 12/14/2013 07:15 PM, Cole Robinson wrote:
On 12/11/2013 03:33 PM, Michele Paolino wrote:
In libvirt, the default error message length is 1024 bytes. This is not enough for qemu to print long error messages such as the list of supported ARM machine models (more than 1700 chars). This is raised when the machine entry in the XML file is wrong, but there may be now or in future other verbose error messages. The above patch enables libvirt to print error messages >1024 for qemu.
Signed-off-by: Michele Paolino <m.paolino@virtualopensystems.com> --- src/qemu/qemu_process.c | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index bd9546e..c2e2136 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1904,10 +1904,25 @@ cleanup: * a possible read of the fd in the monitor code doesn't influence this * error delivery option */ ignore_value(lseek(logfd, pos, SEEK_SET)); - qemuProcessReadLog(logfd, buf + len, buf_size - len - 1, 0, true); - virReportError(VIR_ERR_INTERNAL_ERROR, - _("process exited while connecting to monitor: %s"), - buf); + len = qemuProcessReadLog(logfd, buf + len, buf_size - len - 1, 0, true); + + /* virReportError error buffer is limited to 1024 byte*/ + if (len < 1024){ + virReportError(VIR_ERR_INTERNAL_ERROR, + _("process exited while connecting to monitor: %s"), + buf); + } else { + if (STRPREFIX(buf, "Supported machines are:")) + virReportError(VIR_ERR_INTERNAL_ERROR, + _("process exited while connecting to monitor:" + "please check machine model")); + else + virReportError(VIR_ERR_INTERNAL_ERROR, + _("process exited while connecting to monitor")); + + VIR_ERROR("%s", buf); + } + ret = -1; }
This kind of error scraping is a slipper slop IMO, and for this particular case I don't think it's even that interesting.
I definitely agree with that.
Libvirt already parses the machine types for each qemu emulator and reports it in virsh capabilities XML. Seems reasonable that we could validate the XML machine type at define time and catch an invalid machine type error long before we even try and start the VM.
Do we really want to refuse to define a particular guest just because the host where we're defining it doesn't currently support the given machine type? That's yet another slippery slope - for example, should we also be validating all the devices in <hostdev> at definition time? I think the proper time to do that validation is at domain start time when we should have the proper qemu binary (and necessary drivers/hardware) available.
I'd say we want to do stuff like this when starting a VM rather than defining it. We have a precedent where we check the count of CPUs supported by the emulator when we start the VM rather than at define time. This allows us to craft a useful error message but doesn't forbid to define a guest and then upgrade the emulator to a version that supports all the stuff necessary. Peter
On Mon, Dec 16, 2013 at 10:35:58AM +0100, Peter Krempa wrote:
On 12/16/13 10:27, Laine Stump wrote:
On 12/14/2013 07:15 PM, Cole Robinson wrote:
On 12/11/2013 03:33 PM, Michele Paolino wrote:
In libvirt, the default error message length is 1024 bytes. This is not enough for qemu to print long error messages such as the list of supported ARM machine models (more than 1700 chars). This is raised when the machine entry in the XML file is wrong, but there may be now or in future other verbose error messages. The above patch enables libvirt to print error messages >1024 for qemu.
Signed-off-by: Michele Paolino <m.paolino@virtualopensystems.com> --- src/qemu/qemu_process.c | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index bd9546e..c2e2136 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1904,10 +1904,25 @@ cleanup: * a possible read of the fd in the monitor code doesn't influence this * error delivery option */ ignore_value(lseek(logfd, pos, SEEK_SET)); - qemuProcessReadLog(logfd, buf + len, buf_size - len - 1, 0, true); - virReportError(VIR_ERR_INTERNAL_ERROR, - _("process exited while connecting to monitor: %s"), - buf); + len = qemuProcessReadLog(logfd, buf + len, buf_size - len - 1, 0, true); + + /* virReportError error buffer is limited to 1024 byte*/ + if (len < 1024){ + virReportError(VIR_ERR_INTERNAL_ERROR, + _("process exited while connecting to monitor: %s"), + buf); + } else { + if (STRPREFIX(buf, "Supported machines are:")) + virReportError(VIR_ERR_INTERNAL_ERROR, + _("process exited while connecting to monitor:" + "please check machine model")); + else + virReportError(VIR_ERR_INTERNAL_ERROR, + _("process exited while connecting to monitor")); + + VIR_ERROR("%s", buf); + } + ret = -1; }
This kind of error scraping is a slipper slop IMO, and for this particular case I don't think it's even that interesting.
I definitely agree with that.
Libvirt already parses the machine types for each qemu emulator and reports it in virsh capabilities XML. Seems reasonable that we could validate the XML machine type at define time and catch an invalid machine type error long before we even try and start the VM.
Do we really want to refuse to define a particular guest just because the host where we're defining it doesn't currently support the given machine type? That's yet another slippery slope - for example, should we also be validating all the devices in <hostdev> at definition time? I think the proper time to do that validation is at domain start time when we should have the proper qemu binary (and necessary drivers/hardware) available.
I'd say we want to do stuff like this when starting a VM rather than defining it. We have a precedent where we check the count of CPUs supported by the emulator when we start the VM rather than at define time. This allows us to craft a useful error message but doesn't forbid to define a guest and then upgrade the emulator to a version that supports all the stuff necessary.
Agreed, doing it at start time is best IMHO. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 12/16/2013 04:27 AM, Laine Stump wrote:
On 12/14/2013 07:15 PM, Cole Robinson wrote:
On 12/11/2013 03:33 PM, Michele Paolino wrote:
In libvirt, the default error message length is 1024 bytes. This is not enough for qemu to print long error messages such as the list of supported ARM machine models (more than 1700 chars). This is raised when the machine entry in the XML file is wrong, but there may be now or in future other verbose error messages. The above patch enables libvirt to print error messages >1024 for qemu.
Signed-off-by: Michele Paolino <m.paolino@virtualopensystems.com> --- src/qemu/qemu_process.c | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index bd9546e..c2e2136 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1904,10 +1904,25 @@ cleanup: * a possible read of the fd in the monitor code doesn't influence this * error delivery option */ ignore_value(lseek(logfd, pos, SEEK_SET)); - qemuProcessReadLog(logfd, buf + len, buf_size - len - 1, 0, true); - virReportError(VIR_ERR_INTERNAL_ERROR, - _("process exited while connecting to monitor: %s"), - buf); + len = qemuProcessReadLog(logfd, buf + len, buf_size - len - 1, 0, true); + + /* virReportError error buffer is limited to 1024 byte*/ + if (len < 1024){ + virReportError(VIR_ERR_INTERNAL_ERROR, + _("process exited while connecting to monitor: %s"), + buf); + } else { + if (STRPREFIX(buf, "Supported machines are:")) + virReportError(VIR_ERR_INTERNAL_ERROR, + _("process exited while connecting to monitor:" + "please check machine model")); + else + virReportError(VIR_ERR_INTERNAL_ERROR, + _("process exited while connecting to monitor")); + + VIR_ERROR("%s", buf); + } + ret = -1; }
This kind of error scraping is a slipper slop IMO, and for this particular case I don't think it's even that interesting.
I definitely agree with that.
Libvirt already parses the machine types for each qemu emulator and reports it in virsh capabilities XML. Seems reasonable that we could validate the XML machine type at define time and catch an invalid machine type error long before we even try and start the VM.
Do we really want to refuse to define a particular guest just because the host where we're defining it doesn't currently support the given machine type? That's yet another slippery slope - for example, should we also be validating all the devices in <hostdev> at definition time? I think the proper time to do that validation is at domain start time when we should have the proper qemu binary (and necessary drivers/hardware) available.
My suggestion for define time was because IIRC that's where we do the capabilities arch + os type + domain type validation as well. Doing validation at that level is nice because we don't need to duplicate it in each driver, but it also sucks when qemu is accidentally uninstalled and libvirt is restarted, it now refuses to parse all those pre-existing qemu/kvm configs, and virsh list --all is empty to the great confusion of users. - Cole
On Mon, Dec 16, 2013 at 09:50:30AM -0500, Cole Robinson wrote:
On 12/16/2013 04:27 AM, Laine Stump wrote:
On 12/14/2013 07:15 PM, Cole Robinson wrote:
On 12/11/2013 03:33 PM, Michele Paolino wrote:
In libvirt, the default error message length is 1024 bytes. This is not enough for qemu to print long error messages such as the list of supported ARM machine models (more than 1700 chars). This is raised when the machine entry in the XML file is wrong, but there may be now or in future other verbose error messages. The above patch enables libvirt to print error messages >1024 for qemu.
Signed-off-by: Michele Paolino <m.paolino@virtualopensystems.com> --- src/qemu/qemu_process.c | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index bd9546e..c2e2136 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1904,10 +1904,25 @@ cleanup: * a possible read of the fd in the monitor code doesn't influence this * error delivery option */ ignore_value(lseek(logfd, pos, SEEK_SET)); - qemuProcessReadLog(logfd, buf + len, buf_size - len - 1, 0, true); - virReportError(VIR_ERR_INTERNAL_ERROR, - _("process exited while connecting to monitor: %s"), - buf); + len = qemuProcessReadLog(logfd, buf + len, buf_size - len - 1, 0, true); + + /* virReportError error buffer is limited to 1024 byte*/ + if (len < 1024){ + virReportError(VIR_ERR_INTERNAL_ERROR, + _("process exited while connecting to monitor: %s"), + buf); + } else { + if (STRPREFIX(buf, "Supported machines are:")) + virReportError(VIR_ERR_INTERNAL_ERROR, + _("process exited while connecting to monitor:" + "please check machine model")); + else + virReportError(VIR_ERR_INTERNAL_ERROR, + _("process exited while connecting to monitor")); + + VIR_ERROR("%s", buf); + } + ret = -1; }
This kind of error scraping is a slipper slop IMO, and for this particular case I don't think it's even that interesting.
I definitely agree with that.
Libvirt already parses the machine types for each qemu emulator and reports it in virsh capabilities XML. Seems reasonable that we could validate the XML machine type at define time and catch an invalid machine type error long before we even try and start the VM.
Do we really want to refuse to define a particular guest just because the host where we're defining it doesn't currently support the given machine type? That's yet another slippery slope - for example, should we also be validating all the devices in <hostdev> at definition time? I think the proper time to do that validation is at domain start time when we should have the proper qemu binary (and necessary drivers/hardware) available.
My suggestion for define time was because IIRC that's where we do the capabilities arch + os type + domain type validation as well.
Doing validation at that level is nice because we don't need to duplicate it in each driver, but it also sucks when qemu is accidentally uninstalled and libvirt is restarted, it now refuses to parse all those pre-existing qemu/kvm configs, and virsh list --all is empty to the great confusion of users.
Yeah, that is actually quite a big problem I'd like us to fix one day by moving that validation out of the define stage into the start stage Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 16/12/2013 16:00, Daniel P. Berrange wrote:
On Mon, Dec 16, 2013 at 09:50:30AM -0500, Cole Robinson wrote:
On 12/16/2013 04:27 AM, Laine Stump wrote:
On 12/14/2013 07:15 PM, Cole Robinson wrote:
On 12/11/2013 03:33 PM, Michele Paolino wrote:
In libvirt, the default error message length is 1024 bytes. This is not enough for qemu to print long error messages such as the list of supported ARM machine models (more than 1700 chars). This is raised when the machine entry in the XML file is wrong, but there may be now or in future other verbose error messages. The above patch enables libvirt to print error messages >1024 for qemu.
Signed-off-by: Michele Paolino <m.paolino@virtualopensystems.com> --- src/qemu/qemu_process.c | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index bd9546e..c2e2136 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1904,10 +1904,25 @@ cleanup: * a possible read of the fd in the monitor code doesn't influence this * error delivery option */ ignore_value(lseek(logfd, pos, SEEK_SET)); - qemuProcessReadLog(logfd, buf + len, buf_size - len - 1, 0, true); - virReportError(VIR_ERR_INTERNAL_ERROR, - _("process exited while connecting to monitor: %s"), - buf); + len = qemuProcessReadLog(logfd, buf + len, buf_size - len - 1, 0, true); + + /* virReportError error buffer is limited to 1024 byte*/ + if (len < 1024){ + virReportError(VIR_ERR_INTERNAL_ERROR, + _("process exited while connecting to monitor: %s"), + buf); + } else { + if (STRPREFIX(buf, "Supported machines are:")) + virReportError(VIR_ERR_INTERNAL_ERROR, + _("process exited while connecting to monitor:" + "please check machine model")); + else + virReportError(VIR_ERR_INTERNAL_ERROR, + _("process exited while connecting to monitor")); + + VIR_ERROR("%s", buf); + } + ret = -1; }
This kind of error scraping is a slipper slop IMO, and for this particular case I don't think it's even that interesting. I definitely agree with that.
Libvirt already parses the machine types for each qemu emulator and reports it in virsh capabilities XML. Seems reasonable that we could validate the XML machine type at define time and catch an invalid machine type error long before we even try and start the VM. Do we really want to refuse to define a particular guest just because the host where we're defining it doesn't currently support the given machine type? That's yet another slippery slope - for example, should we also be validating all the devices in <hostdev> at definition time? I think the proper time to do that validation is at domain start time when we should have the proper qemu binary (and necessary drivers/hardware) available.
My suggestion for define time was because IIRC that's where we do the capabilities arch + os type + domain type validation as well.
Doing validation at that level is nice because we don't need to duplicate it in each driver, but it also sucks when qemu is accidentally uninstalled and libvirt is restarted, it now refuses to parse all those pre-existing qemu/kvm configs, and virsh list --all is empty to the great confusion of users. Indeed. In my opinion start time is the best choice because it's more flexible and gives to the user the possibility to catch a more precise error message. Yeah, that is actually quite a big problem I'd like us to fix one day by moving that validation out of the define stage into the start stage
Daniel
-- *Michele Paolino*, Virtualization R&D Engineer Virtual Open Systems /Open Source KVM Virtualization Developments/ /Multicore Systems Virtualization Porting Services/ Web/:/www.virtualopensystems. <http://www.virtualopensystems.com/>com <http://www.virtualopensystems.com/>
On 12/16/2013 10:00 AM, Daniel P. Berrange wrote:
On Mon, Dec 16, 2013 at 09:50:30AM -0500, Cole Robinson wrote:
On 12/16/2013 04:27 AM, Laine Stump wrote:
On 12/14/2013 07:15 PM, Cole Robinson wrote:
On 12/11/2013 03:33 PM, Michele Paolino wrote:
In libvirt, the default error message length is 1024 bytes. This is not enough for qemu to print long error messages such as the list of supported ARM machine models (more than 1700 chars). This is raised when the machine entry in the XML file is wrong, but there may be now or in future other verbose error messages. The above patch enables libvirt to print error messages >1024 for qemu.
Signed-off-by: Michele Paolino <m.paolino@virtualopensystems.com> --- src/qemu/qemu_process.c | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index bd9546e..c2e2136 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1904,10 +1904,25 @@ cleanup: * a possible read of the fd in the monitor code doesn't influence this * error delivery option */ ignore_value(lseek(logfd, pos, SEEK_SET)); - qemuProcessReadLog(logfd, buf + len, buf_size - len - 1, 0, true); - virReportError(VIR_ERR_INTERNAL_ERROR, - _("process exited while connecting to monitor: %s"), - buf); + len = qemuProcessReadLog(logfd, buf + len, buf_size - len - 1, 0, true); + + /* virReportError error buffer is limited to 1024 byte*/ + if (len < 1024){ + virReportError(VIR_ERR_INTERNAL_ERROR, + _("process exited while connecting to monitor: %s"), + buf); + } else { + if (STRPREFIX(buf, "Supported machines are:")) + virReportError(VIR_ERR_INTERNAL_ERROR, + _("process exited while connecting to monitor:" + "please check machine model")); + else + virReportError(VIR_ERR_INTERNAL_ERROR, + _("process exited while connecting to monitor")); + + VIR_ERROR("%s", buf); + } + ret = -1; }
This kind of error scraping is a slipper slop IMO, and for this particular case I don't think it's even that interesting.
I definitely agree with that.
Libvirt already parses the machine types for each qemu emulator and reports it in virsh capabilities XML. Seems reasonable that we could validate the XML machine type at define time and catch an invalid machine type error long before we even try and start the VM.
Do we really want to refuse to define a particular guest just because the host where we're defining it doesn't currently support the given machine type? That's yet another slippery slope - for example, should we also be validating all the devices in <hostdev> at definition time? I think the proper time to do that validation is at domain start time when we should have the proper qemu binary (and necessary drivers/hardware) available.
My suggestion for define time was because IIRC that's where we do the capabilities arch + os type + domain type validation as well.
Doing validation at that level is nice because we don't need to duplicate it in each driver, but it also sucks when qemu is accidentally uninstalled and libvirt is restarted, it now refuses to parse all those pre-existing qemu/kvm configs, and virsh list --all is empty to the great confusion of users.
Yeah, that is actually quite a big problem I'd like us to fix one day by moving that validation out of the define stage into the start stage
FWIW I filed an upstream bug: https://bugzilla.redhat.com/show_bug.cgi?id=1043572 (yeah, the upstream tracker is kinda/sorta a wasteland. But I have this vague plan of sometime in the next six months giving it a serious cleanup, and possibly even tagging some bugs as 'easyfix' or similar and make some noise about intro level tasks for people interested in contributing. this bug would be a decent fit, though don't let that stop anyone from doing it sooner). - Cole
Guys, as for this patch, is it ACKable? are there suggested modifications? Michele On 16/12/2013 16:00, Daniel P. Berrange wrote:
On Mon, Dec 16, 2013 at 09:50:30AM -0500, Cole Robinson wrote:
On 12/16/2013 04:27 AM, Laine Stump wrote:
On 12/14/2013 07:15 PM, Cole Robinson wrote:
On 12/11/2013 03:33 PM, Michele Paolino wrote:
In libvirt, the default error message length is 1024 bytes. This is not enough for qemu to print long error messages such as the list of supported ARM machine models (more than 1700 chars). This is raised when the machine entry in the XML file is wrong, but there may be now or in future other verbose error messages. The above patch enables libvirt to print error messages >1024 for qemu.
Signed-off-by: Michele Paolino<m.paolino@virtualopensystems.com> --- src/qemu/qemu_process.c | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index bd9546e..c2e2136 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1904,10 +1904,25 @@ cleanup: * a possible read of the fd in the monitor code doesn't influence this * error delivery option */ ignore_value(lseek(logfd, pos, SEEK_SET)); - qemuProcessReadLog(logfd, buf + len, buf_size - len - 1, 0, true); - virReportError(VIR_ERR_INTERNAL_ERROR, - _("process exited while connecting to monitor: %s"), - buf); + len = qemuProcessReadLog(logfd, buf + len, buf_size - len - 1, 0, true); + + /* virReportError error buffer is limited to 1024 byte*/ + if (len < 1024){ + virReportError(VIR_ERR_INTERNAL_ERROR, + _("process exited while connecting to monitor: %s"), + buf); + } else { + if (STRPREFIX(buf, "Supported machines are:")) + virReportError(VIR_ERR_INTERNAL_ERROR, + _("process exited while connecting to monitor:" + "please check machine model")); + else + virReportError(VIR_ERR_INTERNAL_ERROR, + _("process exited while connecting to monitor")); + + VIR_ERROR("%s", buf); + } + ret = -1; }
This kind of error scraping is a slipper slop IMO, and for this particular case I don't think it's even that interesting. I definitely agree with that.
Libvirt already parses the machine types for each qemu emulator and reports it in virsh capabilities XML. Seems reasonable that we could validate the XML machine type at define time and catch an invalid machine type error long before we even try and start the VM. Do we really want to refuse to define a particular guest just because the host where we're defining it doesn't currently support the given machine type? That's yet another slippery slope - for example, should we also be validating all the devices in <hostdev> at definition time? I think the proper time to do that validation is at domain start time when we should have the proper qemu binary (and necessary drivers/hardware) available.
My suggestion for define time was because IIRC that's where we do the capabilities arch + os type + domain type validation as well.
Doing validation at that level is nice because we don't need to duplicate it in each driver, but it also sucks when qemu is accidentally uninstalled and libvirt is restarted, it now refuses to parse all those pre-existing qemu/kvm configs, and virsh list --all is empty to the great confusion of users. Yeah, that is actually quite a big problem I'd like us to fix one day by moving that validation out of the define stage into the start stage
Daniel
-- *Michele Paolino*, Virtualization R&D Engineer Virtual Open Systems /Open Source KVM Virtualization Developments/ /Multicore Systems Virtualization Porting Services/ Web/:/www.virtualopensystems. <http://www.virtualopensystems.com/>com <http://www.virtualopensystems.com/>
participants (5)
-
Cole Robinson -
Daniel P. Berrange -
Laine Stump -
Michele Paolino -
Peter Krempa