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(-)
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.
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/daemon/remote.c b/daemon/remote.c
index 3f7d2d3..a5b87e6 100644
--- a/daemon/remote.c
+++ b/daemon/remote.c
@@ -1342,6 +1342,52 @@ remoteRelayDomainEventBlockThreshold(virConnectPtr conn,
}
+static int
+remoteRelayDomainEventBlockJob3(virConnectPtr conn,
+ virDomainPtr dom,
+ const char *dst,
+ int type,
+ int status,
+ const char *error,
+ void *opaque)
+{
+ daemonClientEventCallbackPtr callback = opaque;
+ remote_domain_event_block_job_3_msg data;
+
+ if (callback->callbackID < 0 ||
+ !remoteRelayDomainEventCheckACL(callback->client, conn, dom))
+ return -1;
+
+ VIR_DEBUG("Relaying domain block job 3 event %s %d %s %i, %i, %s, callback %d",
+ dom->name, dom->id, dst, type, status, NULLSTR(error),
+ callback->callbackID);
+
+ /* build return data */
+ memset(&data, 0, sizeof(data));
+ data.callbackID = callback->callbackID;
+ if (VIR_STRDUP(data.dst, dst) < 0)
+ return -1;
+ if (error) {
+ if (VIR_ALLOC(data.error) < 0 ||
+ VIR_STRDUP(*(data.error), error) < 0)
+ goto error;
+ }
+ data.type = type;
+ data.status = status;
+ make_nonnull_domain(&data.dom, dom);
+
+ remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ REMOTE_PROC_DOMAIN_EVENT_BLOCK_JOB_3,
+ (xdrproc_t)xdr_remote_domain_event_block_job_3_msg, &data);
+ return 0;
+
+ error:
+ VIR_FREE(data.dst);
+ VIR_FREE(data.error);
+
+ return -1;
+}
+
static virConnectDomainEventGenericCallback domainEventCallbacks[] = {
VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventLifecycle),
VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventReboot),
@@ -1368,6 +1414,7 @@ static virConnectDomainEventGenericCallback domainEventCallbacks[] = {
VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventDeviceRemovalFailed),
VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventMetadataChange),
VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventBlockThreshold),
+ VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventBlockJob3),
};
verify(ARRAY_CARDINALITY(domainEventCallbacks) == VIR_DOMAIN_EVENT_ID_LAST);
diff --git a/examples/object-events/event-test.c b/examples/object-events/event-test.c
index a144638..623dbd6 100644
--- a/examples/object-events/event-test.c
+++ b/examples/object-events/event-test.c
@@ -936,6 +936,26 @@ myDomainEventBlockJobCallback(virConnectPtr conn ATTRIBUTE_UNUSED,
static int
+myDomainEventBlockJob3Callback(virConnectPtr conn ATTRIBUTE_UNUSED,
+ virDomainPtr dom,
+ const char *disk,
+ int type,
+ int status,
+ void *opaque,
+ const char *error)
+{
+ const char *eventName = opaque;
+
+ printf("%s EVENT: Domain %s(%d) block job callback '%s' disk '%s', "
+ "type '%s' status '%s' error '%s'",
+ __func__, virDomainGetName(dom), virDomainGetID(dom), eventName,
+ disk, blockJobTypeToStr(type), blockJobStatusToStr(status),
+ NULLSTR(error));
+ return 0;
+}
+
+
+static int
myDomainEventBlockThresholdCallback(virConnectPtr conn ATTRIBUTE_UNUSED,
virDomainPtr dom,
const char *dev,
@@ -1082,6 +1102,7 @@ struct domainEventData domainEvents[] = {
DOMAIN_EVENT(VIR_DOMAIN_EVENT_ID_DEVICE_REMOVAL_FAILED, myDomainEventDeviceRemovalFailedCallback),
DOMAIN_EVENT(VIR_DOMAIN_EVENT_ID_METADATA_CHANGE, myDomainEventMetadataChangeCallback),
DOMAIN_EVENT(VIR_DOMAIN_EVENT_ID_BLOCK_THRESHOLD, myDomainEventBlockThresholdCallback),
+ DOMAIN_EVENT(VIR_DOMAIN_EVENT_ID_BLOCK_JOB_3, myDomainEventBlockJob3Callback),
};
struct storagePoolEventData {
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)
+ * @type: type of block job (virDomainBlockJobType)
+ * @status: status of the operation (virConnectDomainEventBlockJobStatus)
+ * @error: error string
+ * @opaque: application specified data
+ *
+ */
+
+
+typedef void (*virConnectDomainEventBlockJob3Callback)(virConnectPtr conn,
+ virDomainPtr dom,
+ const char *disk,
+ int type,
+ int status,
+ const char *error,
+ void *opaque);
/**
* VIR_DOMAIN_EVENT_CALLBACK:
*
@@ -4405,6 +4427,7 @@ typedef enum {
VIR_DOMAIN_EVENT_ID_DEVICE_REMOVAL_FAILED = 22, /* virConnectDomainEventDeviceRemovalFailedCallback */
VIR_DOMAIN_EVENT_ID_METADATA_CHANGE = 23, /* virConnectDomainEventMetadataChangeCallback */
VIR_DOMAIN_EVENT_ID_BLOCK_THRESHOLD = 24, /* virConnectDomainEventBlockThresholdCallback */
+ VIR_DOMAIN_EVENT_ID_BLOCK_JOB_3 = 25, /* virConnectDomainEventBlockJob3Callback */
# ifdef VIR_ENUM_SENTINELS
VIR_DOMAIN_EVENT_ID_LAST
diff --git a/src/conf/domain_event.c b/src/conf/domain_event.c
index 7baccd5..2ea170d 100644
--- a/src/conf/domain_event.c
+++ b/src/conf/domain_event.c
@@ -145,6 +145,7 @@ struct _virDomainEventBlockJob {
virDomainEvent parent;
char *disk; /* path or dst, depending on event id */
+ char *error;
int type;
int status;
};
@@ -501,6 +502,7 @@ virDomainEventBlockJobDispose(void *obj)
VIR_DEBUG("obj=%p", event);
VIR_FREE(event->disk);
+ VIR_FREE(event->error);
}
static void
@@ -995,7 +997,8 @@ virDomainEventBlockJobNew(int event,
unsigned char *uuid,
const char *disk,
int type,
- int status)
+ int status,
+ const char *error)
{
virDomainEventBlockJobPtr ev;
@@ -1007,7 +1010,8 @@ virDomainEventBlockJobNew(int event,
id, name, uuid)))
return NULL;
- if (VIR_STRDUP(ev->disk, disk) < 0) {
+ if (VIR_STRDUP(ev->disk, disk) < 0 ||
+ VIR_STRDUP(ev->error, error) < 0) {
virObjectUnref(ev);
return NULL;
}
@@ -1025,7 +1029,7 @@ virDomainEventBlockJobNewFromObj(virDomainObjPtr obj,
{
return virDomainEventBlockJobNew(VIR_DOMAIN_EVENT_ID_BLOCK_JOB,
obj->def->id, obj->def->name,
- obj->def->uuid, path, type, status);
+ obj->def->uuid, path, type, status, NULL);
}
virObjectEventPtr
@@ -1036,7 +1040,7 @@ virDomainEventBlockJobNewFromDom(virDomainPtr dom,
{
return virDomainEventBlockJobNew(VIR_DOMAIN_EVENT_ID_BLOCK_JOB,
dom->id, dom->name, dom->uuid,
- path, type, status);
+ path, type, status, NULL);
}
virObjectEventPtr
@@ -1047,7 +1051,7 @@ virDomainEventBlockJob2NewFromObj(virDomainObjPtr obj,
{
return virDomainEventBlockJobNew(VIR_DOMAIN_EVENT_ID_BLOCK_JOB_2,
obj->def->id, obj->def->name,
- obj->def->uuid, dst, type, status);
+ obj->def->uuid, dst, type, status, NULL);
}
virObjectEventPtr
@@ -1058,7 +1062,31 @@ virDomainEventBlockJob2NewFromDom(virDomainPtr dom,
{
return virDomainEventBlockJobNew(VIR_DOMAIN_EVENT_ID_BLOCK_JOB_2,
dom->id, dom->name, dom->uuid,
- dst, type, status);
+ dst, type, status, NULL);
+}
+
+virObjectEventPtr
+virDomainEventBlockJob3NewFromObj(virDomainObjPtr obj,
+ const char *dst,
+ int type,
+ int status,
+ const char *error)
+{
+ return virDomainEventBlockJobNew(VIR_DOMAIN_EVENT_ID_BLOCK_JOB_3,
+ obj->def->id, obj->def->name,
+ obj->def->uuid, dst, type, status, error);
+}
+
+virObjectEventPtr
+virDomainEventBlockJob3NewFromDom(virDomainPtr dom,
+ const char *dst,
+ int type,
+ int status,
+ const char *error)
+{
+ return virDomainEventBlockJobNew(VIR_DOMAIN_EVENT_ID_BLOCK_JOB_3,
+ dom->id, dom->name, dom->uuid,
+ dst, type, status, error);
}
virObjectEventPtr
@@ -1871,6 +1899,20 @@ virDomainEventDispatchDefaultFunc(virConnectPtr conn,
goto cleanup;
}
+ case VIR_DOMAIN_EVENT_ID_BLOCK_JOB_3:
+ {
+ virDomainEventBlockJobPtr blockJobEvent;
+
+ blockJobEvent = (virDomainEventBlockJobPtr)event;
+ ((virConnectDomainEventBlockJob3Callback)cb)(conn, dom,
+ blockJobEvent->disk,
+ blockJobEvent->type,
+ blockJobEvent->status,
+ blockJobEvent->error,
+ cbopaque);
+ goto cleanup;
+ }
+
case VIR_DOMAIN_EVENT_ID_DISK_CHANGE:
{
virDomainEventDiskChangePtr diskChangeEvent;
diff --git a/src/conf/domain_event.h b/src/conf/domain_event.h
index 3992a29..1a0f16b 100644
--- a/src/conf/domain_event.h
+++ b/src/conf/domain_event.h
@@ -138,6 +138,19 @@ virDomainEventBlockJob2NewFromDom(virDomainPtr dom,
int status);
virObjectEventPtr
+virDomainEventBlockJob3NewFromObj(virDomainObjPtr obj,
+ const char *dst,
+ int type,
+ int status,
+ const char *error);
+virObjectEventPtr
+virDomainEventBlockJob3NewFromDom(virDomainPtr dom,
+ const char *dst,
+ int type,
+ int status,
+ const char *error);
+
+virObjectEventPtr
virDomainEventDiskChangeNewFromObj(virDomainObjPtr obj,
const char *oldSrcPath,
const char *newSrcPath,
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 448d962..5f0a212 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -562,6 +562,8 @@ virDomainEventBalloonChangeNewFromDom;
virDomainEventBalloonChangeNewFromObj;
virDomainEventBlockJob2NewFromDom;
virDomainEventBlockJob2NewFromObj;
+virDomainEventBlockJob3NewFromDom;
+virDomainEventBlockJob3NewFromObj;
virDomainEventBlockJobNewFromDom;
virDomainEventBlockJobNewFromObj;
virDomainEventBlockThresholdNewFromDom;
diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
index 0b1616a..5266823 100644
--- a/src/qemu/qemu_blockjob.c
+++ b/src/qemu/qemu_blockjob.c
@@ -70,7 +70,8 @@ qemuBlockJobUpdate(virQEMUDriverPtr driver,
if (status != -1) {
qemuBlockJobEventProcess(driver, vm, disk, asyncJob,
diskPriv->blockJobType,
- diskPriv->blockJobStatus);
+ diskPriv->blockJobStatus,
+ diskPriv->blockJobError);
diskPriv->blockJobStatus = -1;
if (error)
VIR_STEAL_PTR(*error, diskPriv->blockJobError);
@@ -100,10 +101,12 @@ qemuBlockJobEventProcess(virQEMUDriverPtr driver,
virDomainDiskDefPtr disk,
qemuDomainAsyncJob asyncJob,
int type,
- int status)
+ int status,
+ const char *error)
{
virObjectEventPtr event = NULL;
virObjectEventPtr event2 = NULL;
+ virObjectEventPtr event3 = NULL;
const char *path;
virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
virDomainDiskDefPtr persistDisk = NULL;
@@ -123,6 +126,7 @@ qemuBlockJobEventProcess(virQEMUDriverPtr driver,
path = virDomainDiskGetSource(disk);
event = virDomainEventBlockJobNewFromObj(vm, path, type, status);
event2 = virDomainEventBlockJob2NewFromObj(vm, disk->dst, type, status);
+ event3 = virDomainEventBlockJob3NewFromObj(vm, disk->dst, type, status, error);
/* If we completed a block pull or commit, then update the XML
* to match. */
@@ -212,6 +216,7 @@ qemuBlockJobEventProcess(virQEMUDriverPtr driver,
qemuDomainEventQueue(driver, event);
qemuDomainEventQueue(driver, event2);
+ qemuDomainEventQueue(driver, event3);
virObjectUnref(cfg);
}
diff --git a/src/qemu/qemu_blockjob.h b/src/qemu/qemu_blockjob.h
index e71d691..18bcaaa 100644
--- a/src/qemu/qemu_blockjob.h
+++ b/src/qemu/qemu_blockjob.h
@@ -36,7 +36,8 @@ void qemuBlockJobEventProcess(virQEMUDriverPtr driver,
virDomainDiskDefPtr disk,
qemuDomainAsyncJob asyncJob,
int type,
- int status);
+ int status,
+ const char *error);
void qemuBlockJobSyncBegin(virDomainDiskDefPtr disk);
void qemuBlockJobSyncEnd(virQEMUDriverPtr driver,
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 735e810..99a71b3 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -457,6 +457,7 @@ struct qemuProcessEvent {
int action;
int status;
void *data;
+ char *error;
};
typedef struct _qemuDomainLogContext qemuDomainLogContext;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 725b46a..c6626bb 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4715,7 +4715,8 @@ processBlockJobEvent(virQEMUDriverPtr driver,
virDomainObjPtr vm,
char *diskAlias,
int type,
- int status)
+ int status,
+ char *error)
{
virDomainDiskDefPtr disk;
@@ -4728,12 +4729,14 @@ processBlockJobEvent(virQEMUDriverPtr driver,
}
if ((disk = qemuProcessFindDomainDiskByAlias(vm, diskAlias)))
- qemuBlockJobEventProcess(driver, vm, disk, QEMU_ASYNC_JOB_NONE, type, status);
+ qemuBlockJobEventProcess(driver, vm, disk, QEMU_ASYNC_JOB_NONE,
+ type, status, error);
endjob:
qemuDomainObjEndJob(driver, vm);
cleanup:
VIR_FREE(diskAlias);
+ VIR_FREE(error);
}
@@ -4815,7 +4818,8 @@ static void qemuProcessEventHandler(void *data, void *opaque)
processBlockJobEvent(driver, vm,
processEvent->data,
processEvent->action,
- processEvent->status);
+ processEvent->status,
+ processEvent->error);
break;
case QEMU_PROCESS_EVENT_MONITOR_EOF:
processMonitorEOFEvent(driver, vm);
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 4bfad5d..6a3d9bc 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -1008,6 +1008,7 @@ qemuProcessHandleBlockJob(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
virDomainDiskDefPtr disk;
qemuDomainDiskPrivatePtr diskPriv;
char *data = NULL;
+ char *errorCopy = NULL;
virObjectLock(vm);
@@ -1018,13 +1019,14 @@ qemuProcessHandleBlockJob(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
goto error;
diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
+ ignore_value(VIR_STRDUP_QUIET(errorCopy, error));
+
if (diskPriv->blockJobSync) {
/* We have a SYNC API waiting for this event, dispatch it back */
diskPriv->blockJobType = type;
diskPriv->blockJobStatus = status;
VIR_FREE(diskPriv->blockJobError);
- if (error && VIR_STRDUP_QUIET(diskPriv->blockJobError, error) < 0)
- VIR_WARN("Can not pass error message further: %s", error);
+ VIR_STEAL_PTR(diskPriv->blockJobError, errorCopy);
virDomainObjBroadcast(vm);
} else {
/* there is no waiting SYNC API, dispatch the update to a thread */
@@ -1038,6 +1040,7 @@ qemuProcessHandleBlockJob(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
processEvent->vm = vm;
processEvent->action = type;
processEvent->status = status;
+ VIR_STEAL_PTR(processEvent->error, errorCopy);
virObjectRef(vm);
if (virThreadPoolSendJob(driver->workerPool, 0, processEvent) < 0) {
@@ -1047,11 +1050,14 @@ qemuProcessHandleBlockJob(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
}
cleanup:
+ VIR_FREE(errorCopy);
virObjectUnlock(vm);
return 0;
error:
- if (processEvent)
+ if (processEvent) {
VIR_FREE(processEvent->data);
+ VIR_FREE(processEvent->error);
+ }
VIR_FREE(processEvent);
goto cleanup;
}
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index 06719bb..c73461d 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -405,6 +405,11 @@ remoteConnectNotifyEventConnectionClosed(virNetClientProgramPtr prog ATTRIBUTE_U
virNetClientPtr client ATTRIBUTE_UNUSED,
void *evdata, void *opaque);
+static void
+remoteDomainBuildEventBlockJob3(virNetClientProgramPtr prog,
+ virNetClientPtr client,
+ void *evdata, void *opaque);
+
static virNetClientProgramEvent remoteEvents[] = {
{ REMOTE_PROC_DOMAIN_EVENT_LIFECYCLE,
remoteDomainBuildEventLifecycle,
@@ -611,6 +616,10 @@ static virNetClientProgramEvent remoteEvents[] = {
remoteDomainBuildEventBlockThreshold,
sizeof(remote_domain_event_block_threshold_msg),
(xdrproc_t)xdr_remote_domain_event_block_threshold_msg },
+ { REMOTE_PROC_DOMAIN_EVENT_BLOCK_JOB_3,
+ remoteDomainBuildEventBlockJob3,
+ sizeof(remote_domain_event_block_job_3_msg),
+ (xdrproc_t)xdr_remote_domain_event_block_job_3_msg },
};
static void
@@ -5610,6 +5619,31 @@ remoteDomainBuildEventBlockThreshold(virNetClientProgramPtr prog ATTRIBUTE_UNUSE
}
+static void
+remoteDomainBuildEventBlockJob3(virNetClientProgramPtr prog ATTRIBUTE_UNUSED,
+ virNetClientPtr client ATTRIBUTE_UNUSED,
+ void *evdata, void *opaque)
+{
+ virConnectPtr conn = opaque;
+ remote_domain_event_block_job_3_msg *msg = evdata;
+ struct private_data *priv = conn->privateData;
+ virDomainPtr dom;
+ virObjectEventPtr event = NULL;
+
+ dom = get_nonnull_domain(conn, msg->dom);
+ if (!dom)
+ return;
+
+ event = virDomainEventBlockJob3NewFromDom(dom, msg->dst, msg->type,
+ msg->status,
+ msg->error ? *msg->error : NULL);
+
+ virObjectUnref(dom);
+
+ remoteEventQueue(priv, event, msg->callbackID);
+}
+
+
static int
remoteStreamSend(virStreamPtr st,
const char *data,
diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x
index 0aed252..371b640 100644
--- a/src/remote/remote_protocol.x
+++ b/src/remote/remote_protocol.x
@@ -3095,6 +3095,15 @@ struct remote_domain_event_block_job_2_msg {
int status;
};
+struct remote_domain_event_block_job_3_msg {
+ int callbackID;
+ remote_nonnull_domain dom;
+ remote_nonnull_string dst;
+ remote_string error;
+ int type;
+ int status;
+};
+
struct remote_domain_event_block_threshold_msg {
int callbackID;
remote_nonnull_domain dom;
@@ -6120,5 +6129,11 @@ enum remote_procedure {
* @generate: both
* @acl: domain:write
*/
- REMOTE_PROC_DOMAIN_SET_LIFECYCLE_ACTION = 390
+ REMOTE_PROC_DOMAIN_SET_LIFECYCLE_ACTION = 390,
+
+ /**
+ * @generate: none
+ * @acl: none
+ */
+ REMOTE_PROC_DOMAIN_EVENT_BLOCK_JOB_3 = 391
};
diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs
index 59b0ace..b8f4065 100644
--- a/src/remote_protocol-structs
+++ b/src/remote_protocol-structs
@@ -2535,6 +2535,14 @@ struct remote_domain_event_block_job_2_msg {
int type;
int status;
};
+struct remote_domain_event_block_job_3_msg {
+ int callbackID;
+ remote_nonnull_domain dom;
+ remote_nonnull_string dst;
+ remote_string error;
+ int type;
+ int status;
+};
struct remote_domain_event_block_threshold_msg {
int callbackID;
remote_nonnull_domain dom;
@@ -3262,4 +3270,5 @@ enum remote_procedure {
REMOTE_PROC_DOMAIN_MANAGED_SAVE_GET_XML_DESC = 388,
REMOTE_PROC_DOMAIN_MANAGED_SAVE_DEFINE_XML = 389,
REMOTE_PROC_DOMAIN_SET_LIFECYCLE_ACTION = 390,
+ REMOTE_PROC_DOMAIN_EVENT_BLOCK_JOB_3 = 391,
};
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 1e33e82..6377c91 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -13253,6 +13253,29 @@ virshEventBlockThresholdPrint(virConnectPtr conn ATTRIBUTE_UNUSED,
}
+static void
+virshEventBlockJob3Print(virConnectPtr conn ATTRIBUTE_UNUSED,
+ virDomainPtr dom,
+ const char *disk,
+ int type,
+ int status,
+ const char *error,
+ void *opaque)
+{
+ virBuffer buf = VIR_BUFFER_INITIALIZER;
+
+ virBufferAsprintf(&buf, _("event '%s' for domain %s: %s for %s %s, "
+ "error: %s\n"),
+ ((virshDomEventData *) opaque)->cb->name,
+ virDomainGetName(dom),
+ virshDomainBlockJobToString(type),
+ disk,
+ virshDomainBlockJobStatusToString(status),
+ NULLSTR(error));
+ virshEventPrint(opaque, &buf);
+}
+
+
static vshEventCallback vshEventCallbacks[] = {
{ "lifecycle",
VIR_DOMAIN_EVENT_CALLBACK(virshEventLifecyclePrint), },
@@ -13302,6 +13325,8 @@ static vshEventCallback vshEventCallbacks[] = {
VIR_DOMAIN_EVENT_CALLBACK(virshEventMetadataChangePrint), },
{ "block-threshold",
VIR_DOMAIN_EVENT_CALLBACK(virshEventBlockThresholdPrint), },
+ { "block-job-3",
+ VIR_DOMAIN_EVENT_CALLBACK(virshEventBlockJob3Print), },
};
verify(VIR_DOMAIN_EVENT_ID_LAST == ARRAY_CARDINALITY(vshEventCallbacks));
--
1.8.3.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 31.10.2017 15:57, Peter Krempa wrote: > 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. > I was thinking to add block job error event instead of extending generic block job event. My concern was how client will use it. So client wants to print error message somewhere. It receives completed event but len and offset are different so it is a error. Client skips this event and awaits new block job error event to capture error message to proceed. Well this steps do not look too unnatural to me. So I only want to share my concern. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Oct 31, 2017 at 16:15:51 +0300, Nikolay Shirokovskiy wrote: > > > On 31.10.2017 15:57, Peter Krempa wrote: > > 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. > > > > I was thinking to add block job error event instead of extending generic > block job event. My concern was how client will use it. So client wants > to print error message somewhere. It receives completed event but len and > offset are different so it is a error. Client skips this event and awaits The events don't report 'len' and 'offset' and querying is inherently racy. If the block job will fail it will not receive the "completed event" but rather VIR_DOMAIN_EVENT_ID_BLOCK_JOB_2 (or '1') with the status field having VIR_DOMAIN_BLOCK_JOB_FAILED. If the APP registered the job error event, it can ignore this one and report failure once the error event is delivered. In case when the registration failed (e.g.) talking to old libvirtd, it can fail when it gets the first event without reporting an error. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2024 Red Hat, Inc.