[libvirt] [PATCH v2] blockjob: use stable disk string in job event
Peter Krempa
pkrempa at redhat.com
Tue Jun 17 07:58:49 UTC 2014
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 at 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
-------------- 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/20140617/3253d812/attachment-0001.sig>
More information about the libvir-list
mailing list