[libvirt] [PATCH -v2 1/1] esx: Make esxDomainGetVcpusFlags check flags

[John Ferlan]

On 08/15/2018 05:35 AM, Marcos Paulo de Souza wrote:
> Before this patch, esxDomainGetVcpusFlags was not checking the flags
> argument. Current, get and set vcpus was failing in ESX since it was
> checking for "maxSupportedVcpus", and this configuration can be ommited
> by ESXi[1]. Now, if VIR_DOMAIN_VCPU_MAXIMUM is specified in flags argument
> esxDomainGetVcpusFlags the maximum number of vcpus allowed for that VM.
> Otherwise, the current number of vcpus is returned.
> 
> With this patch calls to virDomainSetVcpus, virDomainGetMaxVcpus and
> virDomainGetVcpusFlags to return successfull again.
> 
> [1]:https://pubs.vmware.com/vi-sdk/visdk250/ReferenceGuide/vim.host.Capability.html
> 
> Signed-off-by: Marcos Paulo de Souza <marcos.souza.org at gmail.com>
> ---
> 
>  Changes from v1:
>  * Now we only have one patch instead of two
>  * Change esxDomainGetVcpusFlags in order to check the flags argument, instead
>  *   of changing esxDomainGetMaxVcpus.
> 
>  src/esx/esx_driver.c | 52 ++++++++++++++++++++++++--------------------
>  1 file changed, 28 insertions(+), 24 deletions(-)
> 

Since ESX is not generally well known to regular libvir-list readers,
reviewers, and/or committers - I think CC'g Matthias Bolte to ensure
he's aware of the patch in an area where he knows best is a good idea.
Especially on the ping.

If this was more simple, sure usually one of us regulars would handle
this, but you're making more than just simple changes requiring
knowledge of the underlying system that less of us have.

I've CC'd him on this response...

Still wondering how you sent mail on 8/18/18 (your ping to this thread)
which knew about the freeze on 8/28/18 - I'd say something's strange
with your system/mail config... ;-)  Unless of course you've invented
the time machine!

> diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c
> index c2154799fa..a1ba889326 100644
> --- a/src/esx/esx_driver.c
> +++ b/src/esx/esx_driver.c
> @@ -2547,45 +2547,49 @@ esxDomainGetVcpusFlags(virDomainPtr domain, unsigned int flags)
>  {
>      esxPrivate *priv = domain->conn->privateData;
>      esxVI_String *propertyNameList = NULL;
> -    esxVI_ObjectContent *hostSystem = NULL;
> -    esxVI_DynamicProperty *dynamicProperty = NULL;
> +    esxVI_ObjectContent *obj = NULL;
> +    esxVI_Int *vcpus = NULL;
>  
>      virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
>                    VIR_DOMAIN_VCPU_MAXIMUM, -1);
>  
> -    if (priv->maxVcpus > 0)
> -        return priv->maxVcpus;
> -
>      priv->maxVcpus = -1;
>  
>      if (esxVI_EnsureSession(priv->primary) < 0)
>          return -1;
>  
> -    if (esxVI_String_AppendValueToList(&propertyNameList,
> -                                       "capability.maxSupportedVcpus") < 0 ||
> -        esxVI_LookupHostSystemProperties(priv->primary, propertyNameList,
> -                                         &hostSystem) < 0) {
> -        goto cleanup;
> -    }
>  
> -    for (dynamicProperty = hostSystem->propSet; dynamicProperty;
> -         dynamicProperty = dynamicProperty->_next) {
> -        if (STREQ(dynamicProperty->name, "capability.maxSupportedVcpus")) {
> -            if (esxVI_AnyType_ExpectType(dynamicProperty->val,
> -                                         esxVI_Type_Int) < 0) {
> -                goto cleanup;
> -            }
> +    if (flags & VIR_DOMAIN_VCPU_MAXIMUM) {
> +        if (esxVI_String_AppendValueToList(&propertyNameList,
> +                                           "capability.maxHostSupportedVcpus\0"
> +                                           "capability.maxSupportedVcpus") < 0 ||
> +            esxVI_LookupHostSystemProperties(priv->primary,
> +                                             propertyNameList, &obj) < 0 ||
> +            esxVI_GetInt(obj, "capability.maxSupportedVcpus", &vcpus,
> +                         esxVI_Occurrence_OptionalItem) < 0)
> +            goto cleanup;
>  
> -            priv->maxVcpus = dynamicProperty->val->int32;
> -            break;
> -        } else {
> -            VIR_WARN("Unexpected '%s' property", dynamicProperty->name);
> -        }
> +        if (!vcpus && esxVI_GetInt(obj, "capability.maxHostSupportedVcpus",

Can esxVI_GetInt succeed and return NULL for vcpus?

Even it can, this will cause an issue when we leave the "if" and go to
assign priv->maxVcpus from vcpus->value

Don't believe the "!vcpus &&" is necessary. I see one other usage and
the returned pointer is not checked prior to usage, so I think you're
safe, but in the end Matthias certain knows better than I do.

> +                                   &vcpus, esxVI_Occurrence_RequiredItem) < 0)
> +            goto cleanup;
> +
> +    } else {
> +        if (esxVI_String_AppendValueToList(&propertyNameList,
> +                                          "config.hardware.numCPU\0") < 0 ||

This line needs one extra space of alignment

> +            esxVI_LookupVirtualMachineByUuid(priv->primary, domain->uuid,
> +                                             propertyNameList, &obj,
> +                                             esxVI_Occurrence_RequiredItem) < 0 ||
> +            esxVI_GetInt(obj, "config.hardware.numCPU", &vcpus,
> +                         esxVI_Occurrence_RequiredItem) < 0)

Similarly here w/ !vcpus where it's not checked before using a couple
lines below.

John

> +            goto cleanup;
>      }
>  
> +    priv->maxVcpus = vcpus->value;
> +
>   cleanup:
>      esxVI_String_Free(&propertyNameList);
> -    esxVI_ObjectContent_Free(&hostSystem);
> +    esxVI_ObjectContent_Free(&obj);
> +    esxVI_Int_Free(&vcpus);
>  
>      return priv->maxVcpus;
>  }
>