[libvirt] [PATCH v2] blockjob: use stable disk string in job event
When the block job event was first added, it was for block pull, where the active layer of the disk remains the same name. It was also in a day where we only cared about local files, and so we always had a canonical absolute file name. But two things have changed since then: we now have network disks, where determining a single absolute string does not really make sense; and we have two-phase jobs (copy and active commit) where the name of the active layer changes between the first event (ready, on the old name) and second (complete, on the pivoted name). Adam Litke reported that having an unstable string between events makes life harder for clients. Furthermore, all of our API that operate on a particular disk of a domain accept multiple strings: not only the absolute name of the active layer, but also the destination device name (such as 'vda'). As this latter name is stable, even for network sources, it serves as a better string to supply in block job events. But backwards-compatibility demands that we should not change the name handed to users unless they explicitly request it. Therefore, this patch adds a new event, BLOCK_JOB_2 (alas, I couldn't think of any nicer name), and has to double up on emitting both old-style and new-style events according to what clients have registered for (see also how IOError and IOErrorReason emits double events, but there the difference was a larger struct rather than changed meaning of one of the struct members). Unfortunately, adding a new event isn't something that can easily be broken into pieces, so the commit is rather large. * include/libvirt/libvirt.h.in (virDomainEventID): Add a new id for VIR_DOMAIN_EVENT_ID_BLOCK_JOB_2. (virConnectDomainEventBlockJobCallback): Document new semantics. * src/conf/domain_event.c (_virDomainEventBlockJob): Rename field, to ensure we catch all clients. (virDomainEventBlockJobNew): Add parameter. (virDomainEventBlockJobDispose, virDomainEventBlockJobNew) (virDomainEventBlockJobNewFromObj) (virDomainEventBlockJobNewFromDom) (virDomainEventDispatchDefaultFunc): Adjust clients. (virDomainEventBlockJob2NewFromObj) (virDomainEventBlockJob2NewFromDom): New functions. * src/conf/domain_event.h: Add new prototypes. * src/libvirt_private.syms (domain_event.h): Export new functions. * src/qemu/qemu_driver.c (qemuDomainBlockJobImpl): Generate two different events. * src/qemu/qemu_process.c (qemuProcessHandleBlockJob): Likewise. * src/remote/remote_protocol.x (remote_domain_event_block_job_msg): Adjust client. (remote_domain_event_block_job_2_msg): New struct. (REMOTE_PROC_DOMAIN_EVENT_BLOCK_JOB_2): New RPC. * src/remote/remote_driver.c (remoteDomainBuildEventBlockJobHelper): Adjust client. (remoteDomainBuildEventBlockJob2): New handler. (remoteEvents): Register new event. * daemon/remote.c (remoteRelayDomainEventBlockJob): Adjust client. (remoteRelayDomainEventBlockJob2): New handler. (domainEventCallbacks): Register new event. * tools/virsh-domain.c (vshEventCallbacks): Likewise. (vshEventBlockJobPrint): Adjust client. * src/remote_protocol-structs: Regenerate. Signed-off-by: Eric Blake <eblake@redhat.com> --- v1: https://www.redhat.com/archives/libvir-list/2014-June/msg00675.html Diff from v1: add a new event type, and keep clients of the old event unchanged in what they get in their callback. daemon/remote.c | 47 +++++++++++++++++++++++++++++++++++---- include/libvirt/libvirt.h.in | 18 ++++++++++++--- src/conf/domain_event.c | 52 +++++++++++++++++++++++++++++++++----------- src/conf/domain_event.h | 15 +++++++++++-- src/libvirt_private.syms | 2 ++ src/qemu/qemu_driver.c | 8 ++++++- src/qemu/qemu_process.c | 7 ++++++ src/remote/remote_driver.c | 33 +++++++++++++++++++++++++++- src/remote/remote_protocol.x | 18 +++++++++++++-- src/remote_protocol-structs | 10 ++++++++- tools/virsh-domain.c | 7 ++++-- 11 files changed, 188 insertions(+), 29 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index 34c96c9..56cf84f 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -558,7 +558,7 @@ remoteRelayDomainEventGraphics(virConnectPtr conn, static int remoteRelayDomainEventBlockJob(virConnectPtr conn, virDomainPtr dom, - const char *path, + const char *disk, int type, int status, void *opaque) @@ -571,11 +571,11 @@ remoteRelayDomainEventBlockJob(virConnectPtr conn, return -1; VIR_DEBUG("Relaying domain block job event %s %d %s %i, %i, callback %d", - dom->name, dom->id, path, type, status, callback->callbackID); + dom->name, dom->id, disk, type, status, callback->callbackID); /* build return data */ memset(&data, 0, sizeof(data)); - if (VIR_STRDUP(data.path, path) < 0) + if (VIR_STRDUP(data.disk, disk) < 0) goto error; data.type = type; data.status = status; @@ -596,7 +596,7 @@ remoteRelayDomainEventBlockJob(virConnectPtr conn, return 0; error: - VIR_FREE(data.path); + VIR_FREE(data.disk); return -1; } @@ -931,6 +931,44 @@ remoteRelayDomainEventDeviceRemoved(virConnectPtr conn, } +static int +remoteRelayDomainEventBlockJob2(virConnectPtr conn, + virDomainPtr dom, + const char *disk, + int type, + int status, + void *opaque) +{ + daemonClientEventCallbackPtr callback = opaque; + remote_domain_event_block_job_2_msg data; + + if (callback->callbackID < 0 || + !remoteRelayDomainEventCheckACL(callback->client, conn, dom)) + return -1; + + VIR_DEBUG("Relaying domain block job 2 event %s %d %s %i, %i, callback %d", + dom->name, dom->id, disk, type, status, callback->callbackID); + + /* build return data */ + memset(&data, 0, sizeof(data)); + data.callbackID = callback->callbackID; + if (VIR_STRDUP(data.disk, disk) < 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_2, + (xdrproc_t)xdr_remote_domain_event_block_job_2_msg, &data); + + return 0; + error: + VIR_FREE(data.disk); + return -1; +} + + static virConnectDomainEventGenericCallback domainEventCallbacks[] = { VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventLifecycle), VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventReboot), @@ -948,6 +986,7 @@ static virConnectDomainEventGenericCallback domainEventCallbacks[] = { VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventBalloonChange), VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventPMSuspendDisk), VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventDeviceRemoved), + VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventBlockJob2), }; verify(ARRAY_CARDINALITY(domainEventCallbacks) == VIR_DOMAIN_EVENT_ID_LAST); diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index dc88c40..c36fec3 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -4852,13 +4852,24 @@ typedef enum { * virConnectDomainEventBlockJobCallback: * @conn: connection object * @dom: domain on which the event occurred - * @disk: fully-qualified filename of the affected disk + * @disk: name associated with the affected disk * @type: type of block job (virDomainBlockJobType) * @status: status of the operation (virConnectDomainEventBlockJobStatus) * @opaque: application specified data * - * The callback signature to use when registering for an event of type - * VIR_DOMAIN_EVENT_ID_BLOCK_JOB with virConnectDomainEventRegisterAny() + * The string returned for @disk can be used in any of the libvirt API + * that operate on a particular disk of the domain, and depends on what + * event type was registered with virConnectDomainEventRegisterAny(). + * If the callback was registered using the older type of + * VIR_DOMAIN_EVENT_ID_BLOCK_JOB, then @disk contains the absolute file + * name of the host resource for the active layer of the disk; however, + * this name is unstable (pivoting via block copy or active block commit + * will change which file is active, giving a different name for the two + * events associated with the same job) and cannot be relied on if the + * active layer is associated with a network resource. If the callback + * was registered using the newer type of VIR_DOMAIN_EVENT_ID_BLOCK_JOB_2, + * then @disk will contain the device target shorthand (the <target + * dev='...'/> sub-element, such as "vda"). */ typedef void (*virConnectDomainEventBlockJobCallback)(virConnectPtr conn, virDomainPtr dom, @@ -5062,6 +5073,7 @@ typedef enum { VIR_DOMAIN_EVENT_ID_BALLOON_CHANGE = 13, /* virConnectDomainEventBalloonChangeCallback */ VIR_DOMAIN_EVENT_ID_PMSUSPEND_DISK = 14, /* virConnectDomainEventPMSuspendDiskCallback */ VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED = 15, /* virConnectDomainEventDeviceRemovedCallback */ + VIR_DOMAIN_EVENT_ID_BLOCK_JOB_2 = 16, /* virConnectDomainEventBlockJobCallback */ #ifdef VIR_ENUM_SENTINELS VIR_DOMAIN_EVENT_ID_LAST diff --git a/src/conf/domain_event.c b/src/conf/domain_event.c index b565732..43794d3 100644 --- a/src/conf/domain_event.c +++ b/src/conf/domain_event.c @@ -128,7 +128,7 @@ typedef virDomainEventIOError *virDomainEventIOErrorPtr; struct _virDomainEventBlockJob { virDomainEvent parent; - char *path; + char *disk; int type; int status; }; @@ -364,7 +364,7 @@ virDomainEventBlockJobDispose(void *obj) virDomainEventBlockJobPtr event = obj; VIR_DEBUG("obj=%p", event); - VIR_FREE(event->path); + VIR_FREE(event->disk); } static void @@ -775,10 +775,11 @@ virDomainEventGraphicsNewFromObj(virDomainObjPtr obj, } static virObjectEventPtr -virDomainEventBlockJobNew(int id, +virDomainEventBlockJobNew(int event, + int id, const char *name, unsigned char *uuid, - const char *path, + const char *disk, int type, int status) { @@ -788,11 +789,11 @@ virDomainEventBlockJobNew(int id, return NULL; if (!(ev = virDomainEventNew(virDomainEventBlockJobClass, - VIR_DOMAIN_EVENT_ID_BLOCK_JOB, + event, id, name, uuid))) return NULL; - if (VIR_STRDUP(ev->path, path) < 0) { + if (VIR_STRDUP(ev->disk, disk) < 0) { virObjectUnref(ev); return NULL; } @@ -804,22 +805,46 @@ virDomainEventBlockJobNew(int id, virObjectEventPtr virDomainEventBlockJobNewFromObj(virDomainObjPtr obj, - const char *path, + const char *disk, int type, int status) { - return virDomainEventBlockJobNew(obj->def->id, obj->def->name, - obj->def->uuid, path, type, status); + return virDomainEventBlockJobNew(VIR_DOMAIN_EVENT_ID_BLOCK_JOB, + obj->def->id, obj->def->name, + obj->def->uuid, disk, type, status); } virObjectEventPtr virDomainEventBlockJobNewFromDom(virDomainPtr dom, - const char *path, + const char *disk, int type, int status) { - return virDomainEventBlockJobNew(dom->id, dom->name, dom->uuid, - path, type, status); + return virDomainEventBlockJobNew(VIR_DOMAIN_EVENT_ID_BLOCK_JOB, + dom->id, dom->name, dom->uuid, + disk, type, status); +} + +virObjectEventPtr +virDomainEventBlockJob2NewFromObj(virDomainObjPtr obj, + const char *disk, + int type, + int status) +{ + return virDomainEventBlockJobNew(VIR_DOMAIN_EVENT_ID_BLOCK_JOB_2, + obj->def->id, obj->def->name, + obj->def->uuid, disk, type, status); +} + +virObjectEventPtr +virDomainEventBlockJob2NewFromDom(virDomainPtr dom, + const char *disk, + int type, + int status) +{ + return virDomainEventBlockJobNew(VIR_DOMAIN_EVENT_ID_BLOCK_JOB_2, + dom->id, dom->name, dom->uuid, + disk, type, status); } virObjectEventPtr @@ -1250,12 +1275,13 @@ virDomainEventDispatchDefaultFunc(virConnectPtr conn, goto cleanup; case VIR_DOMAIN_EVENT_ID_BLOCK_JOB: + case VIR_DOMAIN_EVENT_ID_BLOCK_JOB_2: { virDomainEventBlockJobPtr blockJobEvent; blockJobEvent = (virDomainEventBlockJobPtr)event; ((virConnectDomainEventBlockJobCallback)cb)(conn, dom, - blockJobEvent->path, + blockJobEvent->disk, blockJobEvent->type, blockJobEvent->status, cbopaque); diff --git a/src/conf/domain_event.h b/src/conf/domain_event.h index 9c41090..c7b8761 100644 --- a/src/conf/domain_event.h +++ b/src/conf/domain_event.h @@ -117,16 +117,27 @@ virDomainEventControlErrorNewFromObj(virDomainObjPtr obj); virObjectEventPtr virDomainEventBlockJobNewFromObj(virDomainObjPtr obj, - const char *path, + const char *disk, int type, int status); virObjectEventPtr virDomainEventBlockJobNewFromDom(virDomainPtr dom, - const char *path, + const char *disk, int type, int status); virObjectEventPtr +virDomainEventBlockJob2NewFromObj(virDomainObjPtr obj, + const char *disk, + int type, + int status); +virObjectEventPtr +virDomainEventBlockJob2NewFromDom(virDomainPtr dom, + const char *disk, + int type, + int status); + +virObjectEventPtr virDomainEventDiskChangeNewFromObj(virDomainObjPtr obj, const char *oldSrcPath, const char *newSrcPath, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 29a9ed1..9e25b8a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -439,6 +439,8 @@ virDomainXMLOptionNew; # conf/domain_event.h virDomainEventBalloonChangeNewFromDom; virDomainEventBalloonChangeNewFromObj; +virDomainEventBlockJob2NewFromDom; +virDomainEventBlockJob2NewFromObj; virDomainEventBlockJobNewFromDom; virDomainEventBlockJobNewFromObj; virDomainEventControlErrorNewFromDom; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4ab5a7b..ca58d6b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15027,6 +15027,7 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, int ret = -1; bool async = false; virObjectEventPtr event = NULL; + virObjectEventPtr event2 = NULL; int idx; virDomainDiskDefPtr disk; virStorageSourcePtr baseSource = NULL; @@ -15130,11 +15131,14 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, if (mode == BLOCK_JOB_ABORT) { if (!async) { /* Older qemu that lacked async reporting also lacked - * active commit, so we can hardcode the event to pull */ + * active commit, so we can hardcode the event to pull. + * We have to generate two variants of the event. */ int type = VIR_DOMAIN_BLOCK_JOB_TYPE_PULL; int status = VIR_DOMAIN_BLOCK_JOB_CANCELED; event = virDomainEventBlockJobNewFromObj(vm, disk->src->path, type, status); + event2 = virDomainEventBlockJob2NewFromObj(vm, disk->dst, type, + status); } else if (!(flags & VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC)) { while (1) { /* Poll every 50ms */ @@ -15178,6 +15182,8 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, virObjectUnlock(vm); if (event) qemuDomainEventQueue(driver, event); + if (event2) + qemuDomainEventQueue(driver, event2); return ret; } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index e4845ba..f1c0041 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1013,6 +1013,7 @@ qemuProcessHandleBlockJob(qemuMonitorPtr mon ATTRIBUTE_UNUSED, { virQEMUDriverPtr driver = opaque; virObjectEventPtr event = NULL; + virObjectEventPtr event2 = NULL; const char *path; virDomainDiskDefPtr disk; @@ -1020,8 +1021,12 @@ qemuProcessHandleBlockJob(qemuMonitorPtr mon ATTRIBUTE_UNUSED, disk = qemuProcessFindDomainDiskByAlias(vm, diskAlias); if (disk) { + /* Have to generate two variants of the event for old vs. new + * client callbacks */ path = virDomainDiskGetSource(disk); event = virDomainEventBlockJobNewFromObj(vm, path, type, status); + event2 = virDomainEventBlockJob2NewFromObj(vm, disk->dst, type, + status); /* XXX If we completed a block pull or commit, then recompute * the cached backing chain to match. Better would be storing * the chain ourselves rather than reprobing, but this @@ -1048,6 +1053,8 @@ qemuProcessHandleBlockJob(qemuMonitorPtr mon ATTRIBUTE_UNUSED, if (event) qemuDomainEventQueue(driver, event); + if (event2) + qemuDomainEventQueue(driver, event2); return 0; } diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 85fe597..8df35e2 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -321,6 +321,11 @@ remoteDomainBuildEventCallbackDeviceRemoved(virNetClientProgramPtr prog, void *evdata, void *opaque); static void +remoteDomainBuildEventBlockJob2(virNetClientProgramPtr prog, + virNetClientPtr client, + void *evdata, void *opaque); + +static void remoteNetworkBuildEventLifecycle(virNetClientProgramPtr prog ATTRIBUTE_UNUSED, virNetClientPtr client ATTRIBUTE_UNUSED, void *evdata, void *opaque); @@ -467,6 +472,10 @@ static virNetClientProgramEvent remoteEvents[] = { remoteDomainBuildEventCallbackDeviceRemoved, sizeof(remote_domain_event_callback_device_removed_msg), (xdrproc_t)xdr_remote_domain_event_callback_device_removed_msg }, + { REMOTE_PROC_DOMAIN_EVENT_BLOCK_JOB_2, + remoteDomainBuildEventBlockJob2, + sizeof(remote_domain_event_block_job_2_msg), + (xdrproc_t)xdr_remote_domain_event_block_job_2_msg }, }; @@ -5023,7 +5032,7 @@ remoteDomainBuildEventBlockJobHelper(virConnectPtr conn, if (!dom) return; - event = virDomainEventBlockJobNewFromDom(dom, msg->path, msg->type, + event = virDomainEventBlockJobNewFromDom(dom, msg->disk, msg->type, msg->status); virDomainFree(dom); @@ -5048,6 +5057,28 @@ remoteDomainBuildEventCallbackBlockJob(virNetClientProgramPtr prog ATTRIBUTE_UNU remote_domain_event_callback_block_job_msg *msg = evdata; remoteDomainBuildEventBlockJobHelper(conn, &msg->msg, msg->callbackID); } +static void +remoteDomainBuildEventBlockJob2(virNetClientProgramPtr prog ATTRIBUTE_UNUSED, + virNetClientPtr client ATTRIBUTE_UNUSED, + void *evdata, void *opaque) +{ + virConnectPtr conn = opaque; + remote_domain_event_block_job_2_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 = virDomainEventBlockJob2NewFromDom(dom, msg->disk, msg->type, + msg->status); + + virDomainFree(dom); + + remoteEventQueue(priv, event, msg->callbackID); +} static void remoteDomainBuildEventGraphicsHelper(virConnectPtr conn, diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index ab9b83d..95a1437 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -2352,7 +2352,7 @@ struct remote_domain_event_callback_graphics_msg { struct remote_domain_event_block_job_msg { remote_nonnull_domain dom; - remote_nonnull_string path; + remote_nonnull_string disk; int type; int status; }; @@ -2948,6 +2948,14 @@ struct remote_domain_event_callback_device_removed_msg { remote_domain_event_device_removed_msg msg; }; +struct remote_domain_event_block_job_2_msg { + int callbackID; + remote_nonnull_domain dom; + remote_nonnull_string disk; + int type; + int status; +}; + struct remote_connect_get_cpu_model_names_args { remote_nonnull_string arch; int need_results; @@ -5338,5 +5346,11 @@ enum remote_procedure { * @generate: both * @acl: domain:set_time */ - REMOTE_PROC_DOMAIN_SET_TIME = 338 + REMOTE_PROC_DOMAIN_SET_TIME = 338, + + /** + * @generate: none + * @acl: none + */ + REMOTE_PROC_DOMAIN_EVENT_BLOCK_JOB_2 = 339 }; diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index 5b22049..3abc076 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -1797,7 +1797,7 @@ struct remote_domain_event_callback_graphics_msg { }; struct remote_domain_event_block_job_msg { remote_nonnull_domain dom; - remote_nonnull_string path; + remote_nonnull_string disk; int type; int status; }; @@ -2413,6 +2413,13 @@ struct remote_domain_event_callback_device_removed_msg { int callbackID; remote_domain_event_device_removed_msg msg; }; +struct remote_domain_event_block_job_2_msg { + int callbackID; + remote_nonnull_domain dom; + remote_nonnull_string disk; + int type; + int status; +}; struct remote_connect_get_cpu_model_names_args { remote_nonnull_string arch; int need_results; @@ -2802,4 +2809,5 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_FSTHAW = 336, REMOTE_PROC_DOMAIN_GET_TIME = 337, REMOTE_PROC_DOMAIN_SET_TIME = 338, + REMOTE_PROC_DOMAIN_EVENT_BLOCK_JOB_2 = 339, }; diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 6b3dd70..d136862 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -10921,8 +10921,9 @@ vshEventBlockJobPrint(virConnectPtr conn ATTRIBUTE_UNUSED, if (!data->loop && *data->count) return; - vshPrint(data->ctl, _("event 'block-job' for domain %s: %s for %s %s\n"), - virDomainGetName(dom), vshDomainBlockJobToString(type), + vshPrint(data->ctl, _("event '%s' for domain %s: %s for %s %s\n"), + data->cb->name, virDomainGetName(dom), + vshDomainBlockJobToString(type), disk, vshDomainBlockJobStatusToString(status)); (*data->count)++; if (!data->loop) @@ -11049,6 +11050,8 @@ static vshEventCallback vshEventCallbacks[] = { VIR_DOMAIN_EVENT_CALLBACK(vshEventPMChangePrint), }, { "device-removed", VIR_DOMAIN_EVENT_CALLBACK(vshEventDeviceRemovedPrint), }, + { "block-job-2", + VIR_DOMAIN_EVENT_CALLBACK(vshEventBlockJobPrint), }, }; verify(VIR_DOMAIN_EVENT_ID_LAST == ARRAY_CARDINALITY(vshEventCallbacks)); -- 1.9.3
On 06/16/14 23:58, Eric Blake wrote:
When the block job event was first added, it was for block pull, where the active layer of the disk remains the same name. It was also in a day where we only cared about local files, and so we always had a canonical absolute file name. But two things have changed since then: we now have network disks, where determining a single absolute string does not really make sense; and we have two-phase jobs (copy and active commit) where the name of the active layer changes between the first event (ready, on the old name) and second (complete, on the pivoted name).
Adam Litke reported that having an unstable string between events makes life harder for clients. Furthermore, all of our API that operate on a particular disk of a domain accept multiple strings: not only the absolute name of the active layer, but also the destination device name (such as 'vda'). As this latter name is stable, even for network sources, it serves as a better string to supply in block job events.
But backwards-compatibility demands that we should not change the name handed to users unless they explicitly request it. Therefore, this patch adds a new event, BLOCK_JOB_2 (alas, I couldn't think of any nicer name), and has to double up on emitting both old-style and new-style events according to what clients have registered for (see also how IOError and IOErrorReason emits double events, but there the difference was a larger struct rather than changed meaning of one of the struct members).
Unfortunately, adding a new event isn't something that can easily be broken into pieces, so the commit is rather large.
* include/libvirt/libvirt.h.in (virDomainEventID): Add a new id for VIR_DOMAIN_EVENT_ID_BLOCK_JOB_2. (virConnectDomainEventBlockJobCallback): Document new semantics. * src/conf/domain_event.c (_virDomainEventBlockJob): Rename field, to ensure we catch all clients. (virDomainEventBlockJobNew): Add parameter. (virDomainEventBlockJobDispose, virDomainEventBlockJobNew) (virDomainEventBlockJobNewFromObj) (virDomainEventBlockJobNewFromDom) (virDomainEventDispatchDefaultFunc): Adjust clients. (virDomainEventBlockJob2NewFromObj) (virDomainEventBlockJob2NewFromDom): New functions. * src/conf/domain_event.h: Add new prototypes. * src/libvirt_private.syms (domain_event.h): Export new functions. * src/qemu/qemu_driver.c (qemuDomainBlockJobImpl): Generate two different events. * src/qemu/qemu_process.c (qemuProcessHandleBlockJob): Likewise. * src/remote/remote_protocol.x (remote_domain_event_block_job_msg): Adjust client. (remote_domain_event_block_job_2_msg): New struct. (REMOTE_PROC_DOMAIN_EVENT_BLOCK_JOB_2): New RPC. * src/remote/remote_driver.c (remoteDomainBuildEventBlockJobHelper): Adjust client. (remoteDomainBuildEventBlockJob2): New handler. (remoteEvents): Register new event. * daemon/remote.c (remoteRelayDomainEventBlockJob): Adjust client. (remoteRelayDomainEventBlockJob2): New handler. (domainEventCallbacks): Register new event. * tools/virsh-domain.c (vshEventCallbacks): Likewise. (vshEventBlockJobPrint): Adjust client. * src/remote_protocol-structs: Regenerate.
Signed-off-by: Eric Blake <eblake@redhat.com> ---
v1: https://www.redhat.com/archives/libvir-list/2014-June/msg00675.html Diff from v1: add a new event type, and keep clients of the old event unchanged in what they get in their callback.
daemon/remote.c | 47 +++++++++++++++++++++++++++++++++++---- include/libvirt/libvirt.h.in | 18 ++++++++++++--- src/conf/domain_event.c | 52 +++++++++++++++++++++++++++++++++----------- src/conf/domain_event.h | 15 +++++++++++-- src/libvirt_private.syms | 2 ++ src/qemu/qemu_driver.c | 8 ++++++- src/qemu/qemu_process.c | 7 ++++++ src/remote/remote_driver.c | 33 +++++++++++++++++++++++++++- src/remote/remote_protocol.x | 18 +++++++++++++-- src/remote_protocol-structs | 10 ++++++++- tools/virsh-domain.c | 7 ++++-- 11 files changed, 188 insertions(+), 29 deletions(-)
diff --git a/daemon/remote.c b/daemon/remote.c index 34c96c9..56cf84f 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -558,7 +558,7 @@ remoteRelayDomainEventGraphics(virConnectPtr conn, static int remoteRelayDomainEventBlockJob(virConnectPtr conn, virDomainPtr dom, - const char *path, + const char *disk, int type, int status, void *opaque)
The original event will still return the path, so I think this hunk should be dropped.
@@ -571,11 +571,11 @@ remoteRelayDomainEventBlockJob(virConnectPtr conn, return -1;
VIR_DEBUG("Relaying domain block job event %s %d %s %i, %i, callback %d", - dom->name, dom->id, path, type, status, callback->callbackID); + dom->name, dom->id, disk, type, status, callback->callbackID);
/* build return data */ memset(&data, 0, sizeof(data)); - if (VIR_STRDUP(data.path, path) < 0) + if (VIR_STRDUP(data.disk, disk) < 0) goto error; data.type = type; data.status = status;
Same here.
@@ -596,7 +596,7 @@ remoteRelayDomainEventBlockJob(virConnectPtr conn,
return 0; error: - VIR_FREE(data.path); + VIR_FREE(data.disk); return -1; }
This one too. [...]
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index dc88c40..c36fec3 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -4852,13 +4852,24 @@ typedef enum { * virConnectDomainEventBlockJobCallback: * @conn: connection object * @dom: domain on which the event occurred - * @disk: fully-qualified filename of the affected disk + * @disk: name associated with the affected disk
The two possible values stored here should also be mentioned here or a note to read the text below. I would have skipped the blob below if I'd find something that looks relevant here.
* @type: type of block job (virDomainBlockJobType) * @status: status of the operation (virConnectDomainEventBlockJobStatus) * @opaque: application specified data * - * The callback signature to use when registering for an event of type - * VIR_DOMAIN_EVENT_ID_BLOCK_JOB with virConnectDomainEventRegisterAny() + * The string returned for @disk can be used in any of the libvirt API + * that operate on a particular disk of the domain, and depends on what + * event type was registered with virConnectDomainEventRegisterAny(). + * If the callback was registered using the older type of + * VIR_DOMAIN_EVENT_ID_BLOCK_JOB, then @disk contains the absolute file + * name of the host resource for the active layer of the disk; however, + * this name is unstable (pivoting via block copy or active block commit + * will change which file is active, giving a different name for the two + * events associated with the same job) and cannot be relied on if the + * active layer is associated with a network resource. If the callback + * was registered using the newer type of VIR_DOMAIN_EVENT_ID_BLOCK_JOB_2, + * then @disk will contain the device target shorthand (the <target + * dev='...'/> sub-element, such as "vda"). */ typedef void (*virConnectDomainEventBlockJobCallback)(virConnectPtr conn, virDomainPtr dom,
diff --git a/src/conf/domain_event.c b/src/conf/domain_event.c index b565732..43794d3 100644 --- a/src/conf/domain_event.c +++ b/src/conf/domain_event.c @@ -128,7 +128,7 @@ typedef virDomainEventIOError *virDomainEventIOErrorPtr; struct _virDomainEventBlockJob { virDomainEvent parent;
- char *path; + char *disk;
Fair enough, this part is common, so changing the name to disk is fine.
int type; int status; };
[...]
@@ -775,10 +775,11 @@ virDomainEventGraphicsNewFromObj(virDomainObjPtr obj, }
static virObjectEventPtr -virDomainEventBlockJobNew(int id, +virDomainEventBlockJobNew(int event, + int id, const char *name, unsigned char *uuid, - const char *path, + const char *disk,
And also here it's fine.
int type, int status) {
[...]
@@ -804,22 +805,46 @@ virDomainEventBlockJobNew(int id,
virObjectEventPtr virDomainEventBlockJobNewFromObj(virDomainObjPtr obj, - const char *path, + const char *disk,
Here I'd rather go with "path".
int type, int status) { - return virDomainEventBlockJobNew(obj->def->id, obj->def->name, - obj->def->uuid, path, type, status); + return virDomainEventBlockJobNew(VIR_DOMAIN_EVENT_ID_BLOCK_JOB, + obj->def->id, obj->def->name, + obj->def->uuid, disk, type, status); }
virObjectEventPtr virDomainEventBlockJobNewFromDom(virDomainPtr dom, - const char *path, + const char *disk,
And here too.
int type, int status) { - return virDomainEventBlockJobNew(dom->id, dom->name, dom->uuid, - path, type, status); + return virDomainEventBlockJobNew(VIR_DOMAIN_EVENT_ID_BLOCK_JOB, + dom->id, dom->name, dom->uuid, + disk, type, status); +} + +virObjectEventPtr +virDomainEventBlockJob2NewFromObj(virDomainObjPtr obj, + const char *disk, + int type, + int status) +{ + return virDomainEventBlockJobNew(VIR_DOMAIN_EVENT_ID_BLOCK_JOB_2, + obj->def->id, obj->def->name, + obj->def->uuid, disk, type, status); +} + +virObjectEventPtr +virDomainEventBlockJob2NewFromDom(virDomainPtr dom, + const char *disk, + int type, + int status) +{ + return virDomainEventBlockJobNew(VIR_DOMAIN_EVENT_ID_BLOCK_JOB_2, + dom->id, dom->name, dom->uuid, + disk, type, status); }
virObjectEventPtr
[...]
diff --git a/src/conf/domain_event.h b/src/conf/domain_event.h index 9c41090..c7b8761 100644 --- a/src/conf/domain_event.h +++ b/src/conf/domain_event.h @@ -117,16 +117,27 @@ virDomainEventControlErrorNewFromObj(virDomainObjPtr obj);
virObjectEventPtr virDomainEventBlockJobNewFromObj(virDomainObjPtr obj, - const char *path, + const char *disk, int type, int status); virObjectEventPtr virDomainEventBlockJobNewFromDom(virDomainPtr dom, - const char *path, + const char *disk, int type, int status);
The two above should still be called with the disk path so I'd rather not change the prototype.
virObjectEventPtr +virDomainEventBlockJob2NewFromObj(virDomainObjPtr obj, + const char *disk, + int type, + int status); +virObjectEventPtr +virDomainEventBlockJob2NewFromDom(virDomainPtr dom, + const char *disk, + int type, + int status); + +virObjectEventPtr virDomainEventDiskChangeNewFromObj(virDomainObjPtr obj, const char *oldSrcPath, const char *newSrcPath,
diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index ab9b83d..95a1437 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -2352,7 +2352,7 @@ struct remote_domain_event_callback_graphics_msg {
struct remote_domain_event_block_job_msg { remote_nonnull_domain dom; - remote_nonnull_string path; + remote_nonnull_string disk;
...
int type; int status; };
diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index 5b22049..3abc076 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -1797,7 +1797,7 @@ struct remote_domain_event_callback_graphics_msg { }; struct remote_domain_event_block_job_msg { remote_nonnull_domain dom; - remote_nonnull_string path; + remote_nonnull_string disk;
Again, this will contain the path, not the name.
int type; int status; };
Looks good apart from renaming arguments for the old event. Peter
On 06/17/2014 01:58 AM, Peter Krempa wrote:
On 06/16/14 23:58, Eric Blake wrote:
When the block job event was first added, it was for block pull, where the active layer of the disk remains the same name. It was also in a day where we only cared about local files, and so we always had a canonical absolute file name. But two things have changed since then: we now have network disks, where determining a single absolute string does not really make sense; and we have two-phase jobs (copy and active commit) where the name of the active layer changes between the first event (ready, on the old name) and second (complete, on the pivoted name).
+++ b/daemon/remote.c @@ -558,7 +558,7 @@ remoteRelayDomainEventGraphics(virConnectPtr conn, static int remoteRelayDomainEventBlockJob(virConnectPtr conn, virDomainPtr dom, - const char *path, + const char *disk, int type, int status, void *opaque)
The original event will still return the path, so I think this hunk should be dropped.
True enough - anywhere where there are sibling functions, I can use const char *path for the old one, and const char *dst for the new one, to make it apparent which string to pass; while leaving *disk as the name of the variable for code shared by both events.
+++ b/include/libvirt/libvirt.h.in @@ -4852,13 +4852,24 @@ typedef enum { * virConnectDomainEventBlockJobCallback: * @conn: connection object * @dom: domain on which the event occurred - * @disk: fully-qualified filename of the affected disk + * @disk: name associated with the affected disk
The two possible values stored here should also be mentioned here or a note to read the text below. I would have skipped the blob below if I'd find something that looks relevant here.
Okay, I'll try again in v3. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 06/17/2014 12:36 PM, Eric Blake wrote:
On 06/17/2014 01:58 AM, Peter Krempa wrote:
On 06/16/14 23:58, Eric Blake wrote:
When the block job event was first added, it was for block pull, where the active layer of the disk remains the same name. It was also in a day where we only cared about local files, and so we always had a canonical absolute file name. But two things have changed since then: we now have network disks, where determining a single absolute string does not really make sense; and we have two-phase jobs (copy and active commit) where the name of the active layer changes between the first event (ready, on the old name) and second (complete, on the pivoted name).
The two possible values stored here should also be mentioned here or a note to read the text below. I would have skipped the blob below if I'd find something that looks relevant here.
Okay, I'll try again in v3.
Oh, and Adam pointed out to me off-list that I also need python bindings for the new event to work in libvirt-python. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Peter Krempa