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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]