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

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