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

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.

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.

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 Daniel

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&lt;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 Daniel