[libvirt] [PATCH] blockjob: use stable disk string in job event

[Peter Krempa]

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 at 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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 901 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140616/9706e90e/attachment-0001.sig>