[libvirt] [REPOST PATCH v2 3/9] qemu: Adjust maxparams logic for qemuDomainGetBlockIoTune

[Erik Skultety]

On Mon, Nov 21, 2016 at 06:35:48PM -0500, John Ferlan wrote:
> Rather than using negative logic and setting the maxparams to a lesser
> value based on which capabilities exist, alter the logic to modify the
> maxparams based on a base value plus the found capabilities. Reduces the
> chance that some backported feature produces an incorrect value.
> 
> Signed-off-by: John Ferlan <jferlan at redhat.com>
> ---
>  src/qemu/qemu_driver.c | 23 ++++++++++++++---------
>  1 file changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index d039255..87d219f 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -112,9 +112,12 @@ VIR_LOG_INIT("qemu.qemu_driver");
>  
>  #define QEMU_NB_MEM_PARAM  3
>  
> -#define QEMU_NB_BLOCK_IO_TUNE_PARAM  6
> -#define QEMU_NB_BLOCK_IO_TUNE_PARAM_MAX  13
> -#define QEMU_NB_BLOCK_IO_TUNE_PARAM_MAX_LENGTH 19
> +#define QEMU_NB_BLOCK_IO_TUNE_BASE_PARAMS 6
> +#define QEMU_NB_BLOCK_IO_TUNE_MAX_PARAMS 7
> +#define QEMU_NB_BLOCK_IO_TUNE_LENGTH_PARAMS 6
> +#define QEMU_NB_BLOCK_IO_TUNE_ALL_PARAMS (QEMU_NB_BLOCK_IO_TUNE_BASE_PARAMS + \
> +                                          QEMU_NB_BLOCK_IO_TUNE_MAX_PARAMS + \
> +                                          QEMU_NB_BLOCK_IO_TUNE_LENGTH_PARAMS)
>  
>  #define QEMU_NB_NUMA_PARAM 2
>  
> @@ -17719,7 +17722,7 @@ qemuDomainGetBlockIoTune(virDomainPtr dom,
>      virDomainBlockIoTuneInfo reply;
>      char *device = NULL;
>      int ret = -1;
> -    int maxparams = QEMU_NB_BLOCK_IO_TUNE_PARAM_MAX_LENGTH;
> +    int maxparams;
>  
>      virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
>                    VIR_DOMAIN_AFFECT_CONFIG |
> @@ -17753,11 +17756,13 @@ qemuDomainGetBlockIoTune(virDomainPtr dom,
>              goto endjob;
>          }
>  
> -        if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX))
> -            maxparams = QEMU_NB_BLOCK_IO_TUNE_PARAM;
> -        else if (!virQEMUCapsGet(priv->qemuCaps,
> -                                 QEMU_CAPS_DRIVE_IOTUNE_MAX_LENGTH))
> -            maxparams = QEMU_NB_BLOCK_IO_TUNE_PARAM_MAX;
> +        maxparams = QEMU_NB_BLOCK_IO_TUNE_BASE_PARAMS;
> +        if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX))
> +            maxparams += QEMU_NB_BLOCK_IO_TUNE_MAX_PARAMS;
> +        if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX_LENGTH))
> +            maxparams += QEMU_NB_BLOCK_IO_TUNE_LENGTH_PARAMS;
> +    } else {
> +        maxparams = QEMU_NB_BLOCK_IO_TUNE_ALL_PARAMS;
>      }
>  

I'm curious about the else branch. Can you elaborate more on why do you assume
qemu supports all the features when you retrieve a persistentdef vs livedef?

From what I understand, this is one of the APIs that you have to call twice,
since the RPC layer does not allocate a structure of an appropriate size
automatically. So with the first run, you say, please allocate a structure of
size maxparams equal _ALL_PARAMS (without knowing that qemu actually supports 
all these features) and rerun.
Then during the second run, you run BLOCK_IOTUNE_ASSIGN on all of them, what am
I missing?

Erik

>      if (*nparams == 0) {
> -- 
> 2.7.4
> 
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20161125/63dd7b39/attachment-0001.sig>