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

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;