On 11/15/2011 02:02 AM, Lei Li wrote:
> This patch add new pulic API virDomainSetBlockIoTune and
> virDomainGetBlockIoTune.
>
> Signed-off-by: Lei Li <lilei@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: Zhi Yong Wu <wuzhy@xxxxxxxxxxxxxxxxxx>
> ---
> include/libvirt/libvirt.h.in | 63 +++++++++++++++++++
> src/driver.h | 20 ++++++
> src/libvirt.c | 141 ++++++++++++++++++++++++++++++++++++++++++
> src/libvirt_public.syms | 6 ++
> 4 files changed, 230 insertions(+), 0 deletions(-)
This fails 'make check':
Missing type converters:
virTypedParameterPtr:2 ERROR: failed virDomainGetBlockIoTune
ERROR: failed virDomainSetBlockIoTune
I see that patch 7/8 adds python support, but for 'git bisect' reasons,
we need something in the interim.
>
> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
> index 2ab89f5..dfa8f76 100644
> --- a/include/libvirt/libvirt.h.in
> +++ b/include/libvirt/libvirt.h.in
> @@ -1671,6 +1671,69 @@ int virDomainBlockPull(virDomainPtr dom, const char *path,
> unsigned long bandwidth, unsigned int flags);
>
>
> +/* Block I/O throttling support */
> +
> +/**
> + * VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC:
> + *
> + * Macro for the BlockIoTune tunable weight: it represents the total
> + * bytes per second permitted through a block device, as a ullong.
> + */
> +#define VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC "total_bytes_sec"
Trailing whitespace ('make syntax-check' didn't like this).
> +++ b/src/driver.h
> @@ -740,6 +740,24 @@ typedef int
> (*virDrvDomainBlockPull)(virDomainPtr dom, const char *path,
> unsigned long bandwidth, unsigned int flags);
>
> +/*
> + * Block I/O throttling support
> + */
> +
> +typedef int
> + (*virDrvDomainSetBlockIoTune)(virDomainPtr dom,
> + const char *disk,
This used 'disk' where we have previously used 'path'. In fact, I like
'disk' better, since it fits in better with the idea of 'path to image'
or 'target disk device shorthand', so maybe I should alter the other API
to use disk as well.
> +
> +/**
> + * virDomainSetBlockIoTune:
> + * @dom: pointer to domain object
> + * @disk: Fully-qualified disk name (or device shothand name)
s/shothand/shorthand/
> + * @params: Pointer to blkio parameter objects
> + * @nparams: Number of blkio parameters (this value can be the same or
> + * less than the number of parameters supported)
> + * @flags: An OR'ed set of virDomainModificationImpact
> + *
> + * Change all or a subset of the per-device block IO tunables.
> + *
> + * The @disk 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").
I previously mentioned in one of the earlier revisions that the existing
text doesn't match code, ever since 0.9.5. So I'd first like to apply
these two commits:
https://www.redhat.com/archives/libvir-list/2011-November/msg01280.html
then reword your patch to use the same descriptions.
> +/**
> + * virDomainGetBlockIoTune:
> + * @dom: pointer to domain object
> + * @disk: Fully-qualified disk name(or device shothand name)
> + * @params: Pointer to blkio parameter object
> + * (return value, allocated by the caller)
> + * @nparams: Pointer to number of blkio parameters
> + * @flags: An OR'ed set of virDomainModificationImpact
> + *
> + * Get all block IO tunable parameters for a given device. On input,
> + * @nparams gives the size of the @params array; on output, @nparams
> + * gives how many slots were filled with parameter information, which
> + * might be less but will not exceed the input value.
> + *
> + * As a special case, calling with @params as NULL and @nparams as 0 on
> + * input will cause @nparams on output to contain the number of parameters
> + * supported by the hypervisor. The caller should then allocate @params
> + * array, i.e. (sizeof(@virTypedParameter) * @nparams) bytes and call the API
> + * agiain.
s/agiain/again/
Also, I previously mentioned that I want to be able to pass NULL for
@disk when @nparams is 0, to get a max possible for the hypervisor, in
contrast to a non-NULL @disk giving the number of parameters possible
for that disk. (It's too late to do the same for
virDomainBlockStatsFlags, which you modeled after, but we might as well
get this one right).
> +
> + if (!disk || (nparams == NULL) || (*nparams < 0) ||
> + (params == NULL && *nparams != 0)) {
> + virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> + goto error;
> + }
As such, this logic needs a tweak, and subsequent patches will need a
bit of help to pass NULL over remote protocol and deal with it in the
qemu driver, but I'm okay with doing that work.
> +LIBVIRT_0.9.8 {
> + global:
> + virDomainSetBlockIoTune;
> + virDomainGetBlockIoTune;
Not sorted (call me a stickler for stuff like that...)
Here's what I plan on squashing in; I'll push if my prereq doc
improvements get acked, and I don't find anything else wrong as I go
through the rest of the series.
diff --git i/include/libvirt/libvirt.h.in w/include/libvirt/libvirt.h.in
index db92875..3204e7c 100644
--- i/include/libvirt/libvirt.h.in
+++ w/include/libvirt/libvirt.h.in
@@ -1679,7 +1679,7 @@ int virDomainBlockPull(virDomainPtr dom,
const char *disk,
* Macro for the BlockIoTune tunable weight: it represents the total
* bytes per second permitted through a block device, as a ullong.
*/
-#define VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC "total_bytes_sec"
+#define VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC "total_bytes_sec"
/**
* VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC:
diff --git i/python/generator.py w/python/generator.py
index 71afdb7..00bd94f 100755
--- i/python/generator.py
+++ w/python/generator.py
@@ -470,6 +470,9 @@ skip_function = (
"virNWFilterGetConnect",
"virStoragePoolGetConnect",
"virStorageVolGetConnect",
+
+ "virDomainGetBlockIoTune", # not implemented yet
+ "virDomainSetBlockIoTune", # not implemented yet
)
qemu_skip_function = (
diff --git i/src/libvirt.c w/src/libvirt.c
index 9b59616..a64bc07 100644
--- i/src/libvirt.c
+++ w/src/libvirt.c
@@ -17192,7 +17192,7 @@ error:
/**
* virDomainSetBlockIoTune:
* @dom: pointer to domain object
- * @disk: Fully-qualified disk name (or device shothand name)
+ * @disk: path to the block device, or device shorthand
* @params: Pointer to blkio parameter objects
* @nparams: Number of blkio parameters (this value can be the same or
* less than the number of parameters supported)
@@ -17200,9 +17200,12 @@ error:
*
* Change all or a subset of the per-device block IO tunables.
*
- * The @disk 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 @disk parameter is either an unambiguous source name of the
+ * block device (the <source file='...'/> sub-element, such as
+ * "/path/to/image"), or 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 error, 0 in case of success.
*/
@@ -17219,7 +17222,7 @@ int virDomainSetBlockIoTune(virDomainPtr dom,
virResetLastError();
- if (!VIR_IS_CONNECTED_DOMAIN (dom)) {
+ if (!VIR_IS_CONNECTED_DOMAIN(dom)) {
virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
virDispatchError(NULL);
return -1;
@@ -17258,7 +17261,7 @@ error:
/**
* virDomainGetBlockIoTune:
* @dom: pointer to domain object
- * @disk: Fully-qualified disk name(or device shothand name)
+ * @disk: path to the block device, or device shorthand
* @params: Pointer to blkio parameter object
* (return value, allocated by the caller)
* @nparams: Pointer to number of blkio parameters
@@ -17269,17 +17272,22 @@ error:
* gives how many slots were filled with parameter information, which
* might be less but will not exceed the input value.
*
- * As a special case, calling with @params as NULL and @nparams as 0 on
- * input will cause @nparams on output to contain the number of parameters
- * supported by the hypervisor. The caller should then allocate @params
- * array, i.e. (sizeof(@virTypedParameter) * @nparams) bytes and call
the API
- * agiain.
- *
- * See virDomainGetMemoryParameters() for an equivalent usage example.
+ * As a special case, calling with @params as NULL and @nparams as 0
+ * on input will cause @nparams on output to contain the number of
+ * parameters supported by the hypervisor, either for the given @disk
+ * (note that block devices of different types might support different
+ * parameters), or if @disk is NULL, for all possible disks. The
+ * caller should then allocate @params array,
+ * i.e. (sizeof(@virTypedParameter) * @nparams) bytes and call the API
+ * again. See virDomainGetMemoryParameters() for more details.
*
- * The @disk 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 @disk parameter is either an unambiguous source name of the
+ * block device (the <source file='...'/> sub-element, such as
+ * "/path/to/image"), or 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. This parameter cannot be NULL
+ * unless @nparams is 0 on input.
*
* Returns -1 in case of error, 0 in case of success.
*/
@@ -17292,7 +17300,7 @@ int virDomainGetBlockIoTune(virDomainPtr dom,
virConnectPtr conn;
VIR_DOMAIN_DEBUG(dom, "disk=%p, params=%p, nparams=%d, flags=%x",
- disk, params, (nparams) ? *nparams : -1, flags);
+ NULLSTR(disk), params, (nparams) ? *nparams : -1,
flags);
virResetLastError();
@@ -17302,8 +17310,8 @@ int virDomainGetBlockIoTune(virDomainPtr dom,
return -1;
}
- if (!disk || (nparams == NULL) || (*nparams < 0) ||
- (params == NULL && *nparams != 0)) {
+ if (nparams == NULL || *nparams < 0 ||
+ ((params == NULL || disk == NULL) && *nparams != 0)) {
virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
goto error;
}
diff --git i/src/libvirt_public.syms w/src/libvirt_public.syms
index 29e34d7..e62387c 100644
--- i/src/libvirt_public.syms
+++ w/src/libvirt_public.syms
@@ -500,8 +500,8 @@ LIBVIRT_0.9.7 {
LIBVIRT_0.9.8 {
global:
- virDomainSetBlockIoTune;
virDomainGetBlockIoTune;
+ virDomainSetBlockIoTune;
} LIBVIRT_0.9.7;
# .... define new API here using predicted next version number ....
--
Eric Blake eblake@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list