[libvirt] [PATCH RFC] lib: provide error message in new blockjob event
pkrempa at redhat.com
Tue Oct 31 12:57:14 UTC 2017
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  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 .
> > Hope this one gain more attention.
> >  https://www.redhat.com/archives/libvir-list/2017-October/msg01292.html
> >  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').
> > + * @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
> > + * @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...
Size: 833 bytes
Desc: not available
More information about the libvir-list