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

Re: [libvirt] [PATCH] blockstats: support lookup by path in blockstats



On Tue, Nov 22, 2011 at 04:42:33PM -0700, Eric Blake wrote:
> Commit 89b6284f made it possible to pass either a source name or
> the target device to most API demanding a disk designation, but
> forgot to update the documentation.  It also failed to update
> virDomainBlockStats to take both forms. This patch fixes both the
> documentation and the remaining function.
> 
> Xen continues to use just device shorthand (that is, I did not
> implement path lookup there, since xen does not track a domain_conf
> to quickly tie a path back to the device shorthand).
> 
> * src/libvirt.c (virDomainBlockStats, virDomainBlockStatsFlags)
> (virDomainGetBlockInfo, virDomainBlockPeek)
> (virDomainBlockJobAbort, virDomainGetBlockJobInfo)
> (virDomainBlockJobSetSpeed, virDomainBlockPull): Document
> acceptable disk naming conventions.
> * src/qemu/qemu_driver.c (qemuDomainBlockStats)
> (qemuDomainBlockStatsFlags): Allow lookup by source name.
> * src/test/test_driver.c (testDomainBlockStats): Likewise.
> ---
> 
> I would like to apply this prior to patch 1/8 in Lei's series:
> https://www.redhat.com/archives/libvir-list/2011-November/msg01145.html
> since that patch should be using the same copy-and-paste documentation.
> 
>  src/libvirt.c          |   82 ++++++++++++++++++++++++++++++++++++-----------
>  src/qemu/qemu_driver.c |   20 ++---------
>  src/test/test_driver.c |   11 +-----
>  3 files changed, 69 insertions(+), 44 deletions(-)
> 
> diff --git a/src/libvirt.c b/src/libvirt.c
> index 17e073e..811dde6 100644
> --- a/src/libvirt.c
> +++ b/src/libvirt.c
> @@ -6659,16 +6659,19 @@ error:
>  /**
>   * virDomainBlockStats:
>   * @dom: pointer to the domain object
> - * @path: path to the block device
> + * @path: path to the block device, or device shorthand
>   * @stats: block device stats (returned)
>   * @size: size of stats structure
>   *
>   * This function returns block device (disk) stats for block
>   * devices attached to the domain.
>   *
> - * The path parameter is the name of the block device.  Get this
> - * by calling virDomainGetXMLDesc and finding the <target dev='...'>
> - * attribute within //domain/devices/disk.  (For example, "xvda").
> + * The @path parameter is either the device target shorthand (the
> + * <target dev='...'/> sub-element, such as "xvda"), or (since 0.9.8)
> + * an unambiguous source name of the block device (the <source
> + * file='...'/> sub-element, such as "/path/to/image").  Valid names
> + * can be found by calling virDomainGetXMLDesc() and inspecting
> + * elements within //domain/devices/disk.
>   *
>   * Domains may have more than one block device.  To get stats for
>   * each you should make multiple calls to this function.
> @@ -6719,7 +6722,7 @@ error:
>  /**
>   * virDomainBlockStatsFlags:
>   * @dom: pointer to domain object
> - * @path: path to the block device
> + * @path: path to the block device, or device shorthand
>   * @params: pointer to block stats parameter object
>   *          (return value)
>   * @nparams: pointer to number of block stats; input and output
> @@ -6728,9 +6731,12 @@ error:
>   * This function is to get block stats parameters for block
>   * devices attached to the domain.
>   *
> - * The @path is the name of the block device.  Get this
> - * by calling virDomainGetXMLDesc and finding the <target dev='...'>
> - * attribute within //domain/devices/disk.  (For example, "xvda").
> + * The @path parameter is either the device target shorthand (the
> + * <target dev='...'/> sub-element, such as "xvda"), or (since 0.9.8)
> + * an unambiguous source name of the block device (the <source
> + * file='...'/> sub-element, such as "/path/to/image").  Valid names
> + * can be found by calling virDomainGetXMLDesc() and inspecting
> + * elements within //domain/devices/disk.
>   *
>   * Domains may have more than one block device.  To get stats for
>   * each you should make multiple calls to this function.
> @@ -6927,7 +6933,7 @@ error:
>  /**
>   * virDomainBlockPeek:
>   * @dom: pointer to the domain object
> - * @path: path to the block device
> + * @path: path to the block device, or device shorthand
>   * @offset: offset within block device
>   * @size: size to read
>   * @buffer: return buffer (must be at least size bytes)
> @@ -6946,10 +6952,12 @@ error:
>   * remote case, nor if you don't have sufficient permission.
>   * Hence the need for this call).
>   *
> - * 'path' must be a device or file corresponding to the domain.
> - * In other words it must be the precise string returned in
> - * a <disk><source dev='...'/></disk> from
> - * virDomainGetXMLDesc.
> + * The @path parameter is either an unambiguous source name of the
> + * block device (the <source file='...'/> sub-element, such as
> + * "/path/to/image"), or (since 0.9.5) the device target shorthand
> + * (the <target dev='...'/> sub-element, such as "xvda").  Valid names
> + * can be found by calling virDomainGetXMLDesc() and inspecting
> + * elements within //domain/devices/disk.
>   *
>   * 'offset' and 'size' represent an area which must lie entirely
>   * within the device or file.  'size' may be 0 to test if the
> @@ -7133,16 +7141,24 @@ error:
>  /**
>   * virDomainGetBlockInfo:
>   * @domain: a domain object
> - * @path: path to the block device or file
> + * @path: path to the block device, or device shorthand
>   * @info: pointer to a virDomainBlockInfo structure allocated by the user
>   * @flags: currently unused, pass zero
>   *
>   * Extract information about a domain's block device.
>   *
> + * The @path parameter is either an unambiguous source name of the
> + * block device (the <source file='...'/> sub-element, such as
> + * "/path/to/image"), or (since 0.9.5) the device target shorthand
> + * (the <target dev='...'/> sub-element, such as "xvda").  Valid names
> + * can be found by calling virDomainGetXMLDesc() and inspecting
> + * elements within //domain/devices/disk.
> + *
>   * Returns 0 in case of success and -1 in case of failure.
>   */
>  int
> -virDomainGetBlockInfo(virDomainPtr domain, const char *path, virDomainBlockInfoPtr info, unsigned int flags)
> +virDomainGetBlockInfo(virDomainPtr domain, const char *path,
> +                      virDomainBlockInfoPtr info, unsigned int flags)
>  {
>      virConnectPtr conn;
> 
> @@ -16837,11 +16853,18 @@ error:
>  /**
>   * virDomainBlockJobAbort:
>   * @dom: pointer to domain object
> - * @path: fully-qualified filename of disk
> + * @path: path to the block device, or device shorthand
>   * @flags: currently unused, for future extension
>   *
>   * Cancel the active block job on the given disk.
>   *
> + * The @path parameter is either an unambiguous source name of the
> + * block device (the <source file='...'/> sub-element, such as
> + * "/path/to/image"), or (since 0.9.5) the device target shorthand
> + * (the <target dev='...'/> sub-element, such as "xvda").  Valid names
> + * can be found by calling virDomainGetXMLDesc() and inspecting
> + * elements within //domain/devices/disk.
> + *
>   * Returns -1 in case of failure, 0 when successful.
>   */
>  int virDomainBlockJobAbort(virDomainPtr dom, const char *path,
> @@ -16889,13 +16912,20 @@ error:
>  /**
>   * virDomainGetBlockJobInfo:
>   * @dom: pointer to domain object
> - * @path: fully-qualified filename of disk
> + * @path: path to the block device, or device shorthand
>   * @info: pointer to a virDomainBlockJobInfo structure
>   * @flags: currently unused, for future extension
>   *
>   * Request block job information for the given disk.  If an operation is active
>   * @info will be updated with the current progress.
>   *
> + * The @path parameter is either an unambiguous source name of the
> + * block device (the <source file='...'/> sub-element, such as
> + * "/path/to/image"), or (since 0.9.5) the device target shorthand
> + * (the <target dev='...'/> sub-element, such as "xvda").  Valid names
> + * can be found by calling virDomainGetXMLDesc() and inspecting
> + * elements within //domain/devices/disk.
> + *
>   * Returns -1 in case of failure, 0 when nothing found, 1 when info was found.
>   */
>  int virDomainGetBlockJobInfo(virDomainPtr dom, const char *path,
> @@ -16944,13 +16974,20 @@ error:
>  /**
>   * virDomainBlockJobSetSpeed:
>   * @dom: pointer to domain object
> - * @path: fully-qualified filename of disk
> + * @path: path to the block device, or device shorthand
>   * @bandwidth: specify bandwidth limit in Mbps
>   * @flags: currently unused, for future extension
>   *
>   * Set the maximimum allowable bandwidth that a block job may consume.  If
>   * bandwidth is 0, the limit will revert to the hypervisor default.
>   *
> + * The @path parameter is either an unambiguous source name of the
> + * block device (the <source file='...'/> sub-element, such as
> + * "/path/to/image"), or (since 0.9.5) the device target shorthand
> + * (the <target dev='...'/> sub-element, such as "xvda").  Valid names
> + * can be found by calling virDomainGetXMLDesc() and inspecting
> + * elements within //domain/devices/disk.
> + *
>   * Returns -1 in case of failure, 0 when successful.
>   */
>  int virDomainBlockJobSetSpeed(virDomainPtr dom, const char *path,
> @@ -16999,7 +17036,7 @@ error:
>  /**
>   * virDomainBlockPull:
>   * @dom: pointer to domain object
> - * @path: Fully-qualified filename of disk
> + * @path: path to the block device, or device shorthand
>   * @bandwidth: (optional) specify copy bandwidth limit in Mbps
>   * @flags: currently unused, for future extension
>   *
> @@ -17010,6 +17047,13 @@ error:
>   * the operation can be aborted with virDomainBlockJobAbort().  When finished,
>   * an asynchronous event is raised to indicate the final status.
>   *
> + * The @path parameter is either an unambiguous source name of the
> + * block device (the <source file='...'/> sub-element, such as
> + * "/path/to/image"), or (since 0.9.5) the device target shorthand
> + * (the <target dev='...'/> sub-element, such as "xvda").  Valid names
> + * can be found by calling virDomainGetXMLDesc() and inspecting
> + * elements within //domain/devices/disk.
> + *
>   * The maximum bandwidth (in Mbps) that will be used to do the copy can be
>   * specified with the bandwidth parameter.  If set to 0, libvirt will choose a
>   * suitable default.  Some hypervisors do not support this feature and will
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index fe2ab85..98ce695 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -7082,18 +7082,12 @@ qemuDomainBlockStats(virDomainPtr dom,
>          goto cleanup;
>      }
> 
> -    for (i = 0 ; i < vm->def->ndisks ; i++) {
> -        if (STREQ(path, vm->def->disks[i]->dst)) {
> -            disk = vm->def->disks[i];
> -            break;
> -        }
> -    }
> -
> -    if (!disk) {
> +    if ((i = virDomainDiskIndexByName(vm->def, path, false)) < 0) {
>          qemuReportError(VIR_ERR_INVALID_ARG,
>                          _("invalid path: %s"), path);
>          goto cleanup;
>      }
> +    disk = vm->def->disks[i];
> 
>      if (!disk->info.alias) {
>          qemuReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -7174,18 +7168,12 @@ qemuDomainBlockStatsFlags(virDomainPtr dom,
>      }
> 
>      if (*nparams != 0) {
> -        for (i = 0 ; i < vm->def->ndisks ; i++) {
> -            if (STREQ(path, vm->def->disks[i]->dst)) {
> -                disk = vm->def->disks[i];
> -                break;
> -            }
> -        }
> -
> -        if (!disk) {
> +        if ((i = virDomainDiskIndexByName(vm->def, path, false)) < 0) {
>              qemuReportError(VIR_ERR_INVALID_ARG,
>                              _("invalid path: %s"), path);
>              goto cleanup;
>          }
> +        disk = vm->def->disks[i];
> 
>          if (!disk->info.alias) {
>               qemuReportError(VIR_ERR_INTERNAL_ERROR,
> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> index e6ff696..f365bf4 100644
> --- a/src/test/test_driver.c
> +++ b/src/test/test_driver.c
> @@ -2803,7 +2803,7 @@ static int testDomainBlockStats(virDomainPtr domain,
>      virDomainObjPtr privdom;
>      struct timeval tv;
>      unsigned long long statbase;
> -    int i, found = 0, ret = -1;
> +    int ret = -1;
> 
>      testDriverLock(privconn);
>      privdom = virDomainFindByName(&privconn->domains,
> @@ -2815,14 +2815,7 @@ static int testDomainBlockStats(virDomainPtr domain,
>          goto error;
>      }
> 
> -    for (i = 0 ; i < privdom->def->ndisks ; i++) {
> -        if (STREQ(path, privdom->def->disks[i]->dst)) {
> -            found = 1;
> -            break;
> -        }
> -    }
> -
> -    if (!found) {
> +    if (virDomainDiskIndexByName(privdom->def, path, false) < 0) {
>          testError(VIR_ERR_INVALID_ARG,
>                    _("invalid path: %s"), path);
>          goto error;

  ACK, looks good, and nice cleanup too

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel veillard com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/


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