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