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

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.