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

[Michele Paolino]

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


-- 
*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/>


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20131218/24680710/attachment-0001.htm>