Re: [libvirt] [PATCH] qemu_agent: Report error class at least
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