[libvirt] [PATCH RFC] lib: provide error message in new blockjob event

[Peter Krempa]

On Tue, Oct 31, 2017 at 15:06:36 +0300, Nikolay Shirokovskiy wrote:
> If block job is completed with error qemu additionally provides error message.
> This patch introduces new event VIR_DOMAIN_EVENT_ID_BLOCK_JOB_3 to pass error
> message to client.
> 
> ---
> 
> The patch is applied on top of [1] patch series (not yet pushed though). Looks
> like this patch consists of too much boilerplate code only to pass extra string
> parameter for blockjob event but looks like there is no other way now.
> Especially ugly looking is adding event3 in src/qemu/qemu_blockjob.c.

Yes, the boilerplate is horrible. Complaining won't help though, you
need to send patches.

> 
> Actually there is old RFC for providing error message for blockjob event [2].
> Hope this one gain more attention.
> 
> [1] https://www.redhat.com/archives/libvir-list/2017-October/msg01292.html
> [2] https://www.redhat.com/archives/libvir-list/2016-December/msg00093.html
> 
>  daemon/remote.c                     | 47 ++++++++++++++++++++++++++++++++
>  examples/object-events/event-test.c | 21 +++++++++++++++
>  include/libvirt/libvirt-domain.h    | 23 ++++++++++++++++
>  src/conf/domain_event.c             | 54 ++++++++++++++++++++++++++++++++-----
>  src/conf/domain_event.h             | 13 +++++++++
>  src/libvirt_private.syms            |  2 ++
>  src/qemu/qemu_blockjob.c            |  9 +++++--
>  src/qemu/qemu_blockjob.h            |  3 ++-
>  src/qemu/qemu_domain.h              |  1 +
>  src/qemu/qemu_driver.c              | 10 ++++---
>  src/qemu/qemu_process.c             | 12 ++++++---
>  src/remote/remote_driver.c          | 34 +++++++++++++++++++++++
>  src/remote/remote_protocol.x        | 17 +++++++++++-
>  src/remote_protocol-structs         |  9 +++++++
>  tools/virsh-domain.c                | 25 +++++++++++++++++
>  15 files changed, 264 insertions(+), 16 deletions(-)

[...]

> diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
> index 4048acf..4f942da 100644
> --- a/include/libvirt/libvirt-domain.h
> +++ b/include/libvirt/libvirt-domain.h
> @@ -4363,6 +4363,28 @@ typedef void (*virConnectDomainEventBlockThresholdCallback)(virConnectPtr conn,
>                                                              unsigned long long excess,
>                                                              void *opaque);
>  
> +
> +/**
> + * virConnectDomainEventBlockJob3Callback:
> + * @conn: connection object
> + * @dom: domain on which the event occurred
> + * @disk: name associated with the affected disk (filename or target
> + *        device, depending on how the callback was registered)

This is copypasted from others, but should be fixed here. We should not
allow the same mistake we did for the '1' event where we pass the path.
This event should only report the target name (along with possibly the
layer via the square bracket syntax 'vda[3]').

> + * @type: type of block job (virDomainBlockJobType)
> + * @status: status of the operation (virConnectDomainEventBlockJobStatus)

I'm not quite sure whether I'm a fan of adding a third event that
reports success. I think we are fine with two. This one should probably
fire only on error conditions and thus the status field will be
unnecessary.

> + * @error: error string

This needs more docs. Especially stating that the error string is
free-form and should NOT be ever used for inferring the error cause
since it's bound to change, and is hypervisor specific. Libvirt will not
guarantee that they will not change.

> + * @opaque: application specified data

In general. The usefullnes of this will be limited for human consumption
since we can't guarantee that the errors will not change.

Otherwise we'd probably report the error as an enum value rather than a
string since it's easier to use for higer level APPs.

If human consumption is what you shoot for, I'm okay with this as long
as it's made clear in the docs.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20171031/f6eee4b6/attachment-0001.sig>