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

[Peter Krempa]

On Tue, Oct 31, 2017 at 13:48:01 +0100, Peter Krempa wrote:
> 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.

One more note on this:
For future use we actually should do a error-only event which will also
have an error-type enum value which currently will have only one
possible value and that's a free-form error string. In the future we can
then assign some codes in some cases.
-------------- 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/24380ec1/attachment-0001.sig>