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@xxxxxxxxxx> > --- > > 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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list