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

[Daniel P. Berrange]

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 at 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 :|