[libvirt] [PATCH] qemu_agent: Report error class at least

[Michal Privoznik]

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