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

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.