[libvirt] [PATCH] 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. * include/libvirt/libvirt.h.in (virConnectDomainEventBlockJobCallback): Document altered semantics. * src/conf/domain_event.c (_virDomainEventBlockJob): Rename field, to ensure we catch all clients. (virDomainEventBlockJobDispose, virDomainEventBlockJobNew) (virDomainEventBlockJobNewFromObj) (virDomainEventBlockJobNewFromDom) (virDomainEventDispatchDefaultFunc): Adjust clients. * src/conf/domain_event.h: Likewise. * src/qemu/qemu_driver.c (qemuDomainBlockJobImpl): Likewise. * src/qemu/qemu_process.c (qemuProcessHandleBlockJob): Likewise. * src/remote/remote_protocol.x (remote_domain_event_block_job_msg): Likewise. * src/remote/remote_driver.c (remoteDomainBuildEventBlockJobHelper): Likewise. * daemon/remote.c (remoteRelayDomainEventBlockJob): Likewise. * src/remote_protocol-structs: Regenerate. Signed-off-by: Eric Blake <eblake@redhat.com> --- If you think backward compatibility of old clients that expected and operated on an absolute name is too important to break, I'm open to suggestions on alternatives how to preserve that. We don't have a flag argument during event registration, or I would have used it. Otherwise, I hope this change is okay as-is. daemon/remote.c | 8 ++++---- include/libvirt/libvirt.h.in | 11 ++++++++++- src/conf/domain_event.c | 18 +++++++++--------- src/conf/domain_event.h | 4 ++-- src/qemu/qemu_driver.c | 2 +- src/qemu/qemu_process.c | 4 +--- src/remote/remote_driver.c | 2 +- src/remote/remote_protocol.x | 2 +- src/remote_protocol-structs | 2 +- 9 files changed, 30 insertions(+), 23 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index 34c96c9..ee1bbbc 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; } diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index dc88c40..a983046 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -4852,11 +4852,20 @@ 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 string returned for @disk can be used in any of the libvirt API + * that operate on a particular disk of the domain. In older versions + * of libvirt, this string was the fully-qualified filename of the + * active layer of the disk; but as some events can change which file + * is the active layer, and because network protocols do not + * necessarily have a canonical filename, libvirt 1.2.6 and newer + * instead use the device target shorthand (the <target dev='...'/> + * sub-element, such as "vda"). + * * The callback signature to use when registering for an event of type * VIR_DOMAIN_EVENT_ID_BLOCK_JOB with virConnectDomainEventRegisterAny() */ diff --git a/src/conf/domain_event.c b/src/conf/domain_event.c index b565732..a2d59fd 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 @@ -778,7 +778,7 @@ static virObjectEventPtr virDomainEventBlockJobNew(int id, const char *name, unsigned char *uuid, - const char *path, + const char *disk, int type, int status) { @@ -792,7 +792,7 @@ virDomainEventBlockJobNew(int id, 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 +804,22 @@ 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); + 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); + disk, type, status); } virObjectEventPtr @@ -1255,7 +1255,7 @@ virDomainEventDispatchDefaultFunc(virConnectPtr conn, 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..e107cc4 100644 --- a/src/conf/domain_event.h +++ b/src/conf/domain_event.h @@ -117,12 +117,12 @@ 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); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b4bf561..c866557 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15135,7 +15135,7 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, * active commit, so we can hardcode the event to pull */ int type = VIR_DOMAIN_BLOCK_JOB_TYPE_PULL; int status = VIR_DOMAIN_BLOCK_JOB_CANCELED; - event = virDomainEventBlockJobNewFromObj(vm, disk->src->path, type, + event = virDomainEventBlockJobNewFromObj(vm, disk->dst, type, status); } else if (!(flags & VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC)) { while (1) { diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index e4845ba..68cfd4c 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1013,15 +1013,13 @@ qemuProcessHandleBlockJob(qemuMonitorPtr mon ATTRIBUTE_UNUSED, { virQEMUDriverPtr driver = opaque; virObjectEventPtr event = NULL; - const char *path; virDomainDiskDefPtr disk; virObjectLock(vm); disk = qemuProcessFindDomainDiskByAlias(vm, diskAlias); if (disk) { - path = virDomainDiskGetSource(disk); - event = virDomainEventBlockJobNewFromObj(vm, path, type, status); + event = virDomainEventBlockJobNewFromObj(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 diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 85fe597..8a83a41 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -5023,7 +5023,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); diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 1f9d583..e9a23cf 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..9904f3d 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; }; -- 1.9.3
On 06/14/14 15:49, 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.
* include/libvirt/libvirt.h.in (virConnectDomainEventBlockJobCallback): Document altered semantics. * src/conf/domain_event.c (_virDomainEventBlockJob): Rename field, to ensure we catch all clients. (virDomainEventBlockJobDispose, virDomainEventBlockJobNew) (virDomainEventBlockJobNewFromObj) (virDomainEventBlockJobNewFromDom) (virDomainEventDispatchDefaultFunc): Adjust clients. * src/conf/domain_event.h: Likewise. * src/qemu/qemu_driver.c (qemuDomainBlockJobImpl): Likewise. * src/qemu/qemu_process.c (qemuProcessHandleBlockJob): Likewise. * src/remote/remote_protocol.x (remote_domain_event_block_job_msg): Likewise. * src/remote/remote_driver.c (remoteDomainBuildEventBlockJobHelper): Likewise. * daemon/remote.c (remoteRelayDomainEventBlockJob): Likewise. * src/remote_protocol-structs: Regenerate.
Signed-off-by: Eric Blake <eblake@redhat.com> ---
If you think backward compatibility of old clients that expected and operated on an absolute name is too important to break, I'm open to suggestions on alternatives how to preserve that. We don't have a flag argument during event registration, or I would have used it. Otherwise, I hope this change is okay as-is.
I'm not a big fan of supporting bad design decisions forever thus I'd vote for changing this. On the other hand, somebody might already depend on this and we'd break them badly. I'd definitely want to hear at least one other opinion on this. If the decision will be to not change this event, we might add a different event that will have the disk name as the parameter and will be emitted simultaneously. Code review below.
daemon/remote.c | 8 ++++---- include/libvirt/libvirt.h.in | 11 ++++++++++- src/conf/domain_event.c | 18 +++++++++--------- src/conf/domain_event.h | 4 ++-- src/qemu/qemu_driver.c | 2 +- src/qemu/qemu_process.c | 4 +--- src/remote/remote_driver.c | 2 +- src/remote/remote_protocol.x | 2 +- src/remote_protocol-structs | 2 +- 9 files changed, 30 insertions(+), 23 deletions(-)
Code changes look okay, so ACK if the political question gets cleared. Peter
On Sat, Jun 14, 2014 at 07:49:40AM -0600, Eric Blake wrote:
If you think backward compatibility of old clients that expected and operated on an absolute name is too important to break, I'm open to suggestions on alternatives how to preserve that. We don't have a flag argument during event registration, or I would have used it. Otherwise, I hope this change is okay as-is.
Hmm, I fear this is one of those cases where it seems fine to change the semantics, but is liable to bite us later. The most obvious alternative is to just introduce a new event BLOCK_JOB_EVENT_2 or something less sucky. That still leaves the question of how to generate the old event in the fact of a disk with no local name - we could use empty string '' as the "path" if the disk lacks one. Or we could simply not emit the old style event. I'd probably go for empty string though. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 06/16/2014 09:45 AM, Daniel P. Berrange wrote:
On Sat, Jun 14, 2014 at 07:49:40AM -0600, Eric Blake wrote:
If you think backward compatibility of old clients that expected and operated on an absolute name is too important to break, I'm open to suggestions on alternatives how to preserve that. We don't have a flag argument during event registration, or I would have used it. Otherwise, I hope this change is okay as-is.
Hmm, I fear this is one of those cases where it seems fine to change the semantics, but is liable to bite us later.
The most obvious alternative is to just introduce a new event BLOCK_JOB_EVENT_2 or something less sucky. That still leaves the question of how to generate the old event in the fact of a disk with no local name - we could use empty string '' as the "path" if the disk lacks one. Or we could simply not emit the old style event. I'd probably go for empty string though.
Adam was the one that brought this issue up in the first place. Adam, would you be okay registering for a new event id that guarantees you get a stable name? Libvirt would have to be able to generate both old and new style events (yuck for me to code up, but such is life of back-compat), but Dan has a valid point that the change is subtle enough that it might bite us if we don't have some way to get both the old and the new style event by explicit user request. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 16/06/14 09:52 -0600, Eric Blake wrote:
On 06/16/2014 09:45 AM, Daniel P. Berrange wrote:
On Sat, Jun 14, 2014 at 07:49:40AM -0600, Eric Blake wrote:
If you think backward compatibility of old clients that expected and operated on an absolute name is too important to break, I'm open to suggestions on alternatives how to preserve that. We don't have a flag argument during event registration, or I would have used it. Otherwise, I hope this change is okay as-is.
Hmm, I fear this is one of those cases where it seems fine to change the semantics, but is liable to bite us later.
The most obvious alternative is to just introduce a new event BLOCK_JOB_EVENT_2 or something less sucky. That still leaves the question of how to generate the old event in the fact of a disk with no local name - we could use empty string '' as the "path" if the disk lacks one. Or we could simply not emit the old style event. I'd probably go for empty string though.
Adam was the one that brought this issue up in the first place. Adam, would you be okay registering for a new event id that guarantees you get a stable name?
Yeah, this would be fine for me.
Libvirt would have to be able to generate both old and new style events (yuck for me to code up, but such is life of back-compat), but Dan has a valid point that the change is subtle enough that it might bite us if we don't have some way to get both the old and the new style event by explicit user request.
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
-- Adam Litke
On 06/16/2014 11:58 AM, Adam Litke wrote:
The most obvious alternative is to just introduce a new event BLOCK_JOB_EVENT_2 or something less sucky. That still leaves
Anyone have a suggestion for a better name? I used VIR_DOMAIN_EVENT_ID_BLOCK_JOB_2; and about the only other name I could come up with was the longer VIR_DOMAIN_EVENT_ID_BLOCK_DEVICE_JOB, since we are pinning the disk parameter of the callback to the destination device name.
the question of how to generate the old event in the fact of a disk with no local name - we could use empty string '' as the "path" if the disk lacks one. Or we could simply not emit the old style event. I'd probably go for empty string though.
Adam was the one that brought this issue up in the first place. Adam, would you be okay registering for a new event id that guarantees you get a stable name?
Yeah, this would be fine for me.
Okay, v2 is posted along those lines. https://www.redhat.com/archives/libvir-list/2014-June/msg00748.html -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (4)
-
Adam Litke -
Daniel P. Berrange -
Eric Blake -
Peter Krempa