Re: [libvirt] [PATCH] qemu: add support for error messages greater than 1024 characters

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(a)virtualopensystems.com&gt; >>> --- >>> 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.