Re: [Qemu-devel] [PATCH 2/3] qmp: Make Quorum error events more palatable.

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2319 bytes --]

On 02/21/2014 04:58 PM, Benoît Canet wrote:
> Insert quorum QMP events documentation alphabetically.
> 
> Also change the "ret" errno value by "error" being an strerror() in the
> QUORUM_REPORT_BAD qmp event.
> 
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
>  block/quorum.c             |  5 ++--
>  docs/qmp/qmp-events.txt    | 72 +++++++++++++++++++++++-----------------------
>  tests/qemu-iotests/081.out |  2 +-
>  3 files changed, 40 insertions(+), 39 deletions(-)
> 
> diff --git a/block/quorum.c b/block/quorum.c
> index 6c28239..3ef8eaa 100644
> --- a/block/quorum.c
> +++ b/block/quorum.c
> @@ -200,11 +200,12 @@ static void quorum_report_bad(QuorumAIOCB *acb, char *node_name, int ret)
>  {
>      QObject *data;
>      assert(node_name);
> -    data = qobject_from_jsonf("{ 'ret': %d"
> +    data = qobject_from_jsonf("{ 'error': %s"
>                                ", 'node-name': %s"
>                                ", 'sector-num': %" PRId64
>                                ", 'sectors-count': %d }",
> -                              ret, node_name, acb->sector_num, acb->nb_sectors);
> +                              strerror(ret), node_name, acb->sector_num,

Is ret positive or negative?  Our typical convention is to use -errno
values for return (since we prefer reporting errors as less than 0),
which may mean needing strerror(-ret) here.

Also, I'd suggest making 'error' optional, and only omit it when ret is
non-zero.  Otherwise...

> +{ "event": "QUORUM_REPORT_BAD",
> +     "data": { "error": "Success", "node-name": "1.raw", "sector-num": 345435,

we get these weird-looking "error": "Success" strings.  Look at how
BLOCK_JOB_COMPLETED documents things - I'd just lift that full paragraph:

- "error":    Error message (json-string, optional)
              Only present on failure.  This field contains a human-readable
              error message.  There are no semantics other than that
streaming
              has failed and clients should not try to interpret the error
              string.

(When we eventually add events into qapi-schema.json as a first-class
citizen, it will be listed as '*error':'str')

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]