On 12.04.2012 14:14, Daniel P. Berrange wrote:
> On Thu, Apr 12, 2012 at 02:06:21PM +0200, Michal Privoznik wrote:
>> Currently, qemu GA is not providing 'desc' field for errors like
>> we are used to from qemu monitor. Therefore, we fall back to this
>> general 'unknown error' string. However, GA is reporting 'class' which
>> is not perfect, but much more helpful than generic error string.
>> Thus we should fall back to class firstly and if even no class
>> is presented, then we can fall back to that generic string.
>>
>> Before this patch:
>> virsh # dompmsuspend --target mem f16
>> error: Domain f16 could not be suspended
>> error: internal error unable to execute QEMU command
>> 'guest-suspend-ram': unknown QEMU command error
>>
>> After this patch:
>> virsh # dompmsuspend --target mem f16
>> error: Domain f16 could not be suspended
>> error: internal error unable to execute QEMU command
>> 'guest-suspend-ram': CommandNotFound
>> ---
>> src/qemu/qemu_agent.c | 14 ++++++--------
>> 1 files changed, 6 insertions(+), 8 deletions(-)
>>
>> diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
>> index b759b7f..decfd0e 100644
>> --- a/src/qemu/qemu_agent.c
>> +++ b/src/qemu/qemu_agent.c
>> @@ -1035,19 +1035,17 @@ static const char *
>> qemuAgentStringifyError(virJSONValuePtr error)
>> {
>> const char *klass = virJSONValueObjectGetString(error, "class");
>> - const char *detail = NULL;
>> + const char *detail = virJSONValueObjectGetString(error, "desc");
>>
>> /* The QMP 'desc' field is usually sufficient for our generic
>> - * error reporting needs.
>> + * error reporting needs. However, older agents did not provide
>> + * any 'desc'. Reporting 'class' is not perfect but better
>> + * than bare 'unknown error'.
>> */
>> - if (klass)
>> - detail = virJSONValueObjectGetString(error, "desc");
>> -
>> -
>> - if (!detail)
>> + if (!detail && !klass)
>> detail = "unknown QEMU command error";
>>
>> - return detail;
>> + return detail ? detail : klass;
>> }
>
> You can get a list of all 'class' names that QEMU currently supports
> from qerror.h. As we do for VIR_ERR_XXXXX constants, you should create
> a mapping from QEMU class names, to error message strings.
>
> Even better, share this mapping between the agent & json monitor code
> so we can improve error messages in both cases.
>
> Regards,
> Daniel
I don't think this should be shared; as written in commit message,
in case of json monitor we get 'desc' which fulfills translation
from 'class' to error message. Moreover, the 'desc' field already
contains correct values, e.g.:
#define QERR_UNKNOWN_BLOCK_FORMAT_FEATURE \
"{ 'class': 'UnknownBlockFormatFeature', 'data': { 'device': %s, 'format': %s, 'feature': %s } }"
{"execute":"unknown command"}
{"error": {"class": "CommandNotFound", "desc": "The command unknown command has not been found", "data": {"name": "unknown command"}}}
However, things change rapidly with GA:
{"execute":"unknown command"}
{"error": {"class": "CommandNotFound", "data": {"name": "unknown command"}}}
Therefore I see point in having translation table just for GA.
In fact, such table doesn't need to cover all error codes from qerror.h
only those used by GA:
~/work/qemu.git $ grep -r -o -e QERR_[A-Z_]* qga/ qapi* | cut -d':' -f 2 | sort | uniq
QERR_BUFFER_OVERRUN
QERR_COMMAND_DISABLED
QERR_COMMAND_NOT_FOUND
QERR_FD_NOT_FOUND
QERR_INVALID_PARAMETER
QERR_INVALID_PARAMETER_TYPE
QERR_INVALID_PARAMETER_VALUE
QERR_OPEN_FILE_FAILED
QERR_QGA_COMMAND_FAILED
QERR_QGA_LOGGING_DISABLED
QERR_QMP_BAD_INPUT_OBJECT
QERR_QMP_BAD_INPUT_OBJECT_MEMBER
QERR_QMP_EXTRA_MEMBER
QERR_UNDEFINED_ERROR
QERR_UNSUPPORTED
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list