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

Re: [libvirt] [PATCH v4 5/5] hyperv: Add support for virDomainSetMemory



2017-05-19 22:58 GMT+02:00 Sri Ramanujam <sramanujam datto com>:
> Introduces support for virDomainSetMemory. This also serves an an
> example for how to use the new method invocation API with a more
> complicated method, this time including an EPR and embedded param.
> ---
>  src/hyperv/hyperv_driver.c            | 94 +++++++++++++++++++++++++++++++++++
>  src/hyperv/hyperv_wmi.c               | 51 +++++++++++++++++++
>  src/hyperv/hyperv_wmi.h               | 11 ++++
>  src/hyperv/hyperv_wmi_generator.input | 30 +++++++++++
>  4 files changed, 186 insertions(+)
>
> diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c
> index a01515a..455e1cd 100644
> --- a/src/hyperv/hyperv_driver.c
> +++ b/src/hyperv/hyperv_driver.c
> @@ -1481,6 +1481,98 @@ hypervDomainSendKey(virDomainPtr domain, unsigned int codeset,
>  }
>
>
> +static int
> +hypervDomainSetMemoryFlags(virDomainPtr domain, unsigned long memory,
> +        unsigned int flags)
> +{
> +    int result = -1;
> +    char uuid_string[VIR_UUID_STRING_BUFLEN];
> +    hypervPrivate *priv = domain->conn->privateData;
> +    char *memory_str = NULL;
> +    hypervInvokeParamsListPtr params = NULL;
> +    unsigned long memory_mb = memory / 1024;

Use VIR_DIV_UP(memory, 1024) here, to round up to full megabytes.

> +    Msvm_VirtualSystemSettingData *vssd = NULL;
> +    Msvm_MemorySettingData *memsd = NULL;
> +    virBuffer eprQuery = VIR_BUFFER_INITIALIZER;
> +    virHashTablePtr memResource = NULL;
> +
> +    virCheckFlags(0, -1);
> +
> +    /* memory has to be a multiple of 2; round up if necessary */
> +    if (memory_mb % 2) memory_mb++;
> +
> +    if (virAsprintf(&memory_str, "%lu", memory_mb) < 0)
> +        goto cleanup;
> +
> +    virUUIDFormat(domain->uuid, uuid_string);
> +
> +    if (hypervGetMsvmVirtualSystemSettingDataFromUUID(priv, uuid_string, &vssd) < 0)
> +        goto cleanup;
> +
> +    if (hypervGetMsvmMemorySettingDataFromVSSD(priv, vssd->data.common->InstanceID,
> +                &memsd) < 0)
> +        goto cleanup;
> +
> +    if (priv->wmiVersion == HYPERV_WMI_VERSION_V1) {
> +        params = hypervCreateInvokeParamsList(priv, "ModifyVirtualSystemResources",
> +                MSVM_VIRTUALSYSTEMMANAGEMENTSERVICE_SELECTOR,
> +                Msvm_VirtualSystemManagementService_WmiInfo);

Shouldn't you check for params == NULL here?

> +
> +        virBufferAddLit(&eprQuery, MSVM_COMPUTERSYSTEM_WQL_SELECT);
> +        virBufferAsprintf(&eprQuery, "where Name = \"%s\"", uuid_string);
> +
> +        if (hypervAddEprParam(params, "ComputerSystem", priv, &eprQuery,
> +                    Msvm_ComputerSystem_WmiInfo) < 0)
> +            goto cleanup;
> +    } else if (priv->wmiVersion == HYPERV_WMI_VERSION_V2) {
> +        params = hypervCreateInvokeParamsList(priv, "ModifyResourceSettings",
> +                MSVM_VIRTUALSYSTEMMANAGEMENTSERVICE_SELECTOR,
> +                Msvm_VirtualSystemManagementService_WmiInfo);

Shouldn't you check for params == NULL here?

> +    }
> +
> +    memResource = hypervCreateEmbeddedParam(priv, Msvm_MemorySettingData_WmiInfo);
> +    if (memResource == NULL)
> +        goto cleanup;
> +
> +    if (hypervSetEmbeddedProperty(memResource, "VirtualQuantity", memory_str) < 0)

memResource leaks here.

> +        goto cleanup;
> +
> +    if (hypervSetEmbeddedProperty(memResource, "InstanceID",
> +                memsd->data.common->InstanceID) < 0)

memResource leaks here.

> +        goto cleanup;
> +
> +    if (priv->wmiVersion == HYPERV_WMI_VERSION_V1) {
> +        if (hypervAddEmbeddedParam(params, priv, "ResourceSettingData",
> +                    memResource, Msvm_MemorySettingData_WmiInfo) < 0)

memResource leaks here.

> +            goto cleanup;
> +
> +    } else if (priv->wmiVersion == HYPERV_WMI_VERSION_V2) {
> +        if (hypervAddEmbeddedParam(params, priv, "ResourceSettings",
> +                    memResource, Msvm_MemorySettingData_WmiInfo) < 0)

memResource leaks here.

> +            goto cleanup;
> +    }

Only now after an successful hypervAddEmbeddedParam call memResource
will not leak anymore because the hypervInvokeMethod will free it as
part of the hypervInvokeParamsListPtr cleanup.

I suggest adding a hypervFreeEmbeddedParam function that does the
hashtable cleanup and call it from hypervFreeInvokeParams instead of
doing the hashtable cleanup directly in hypervFreeInvokeParams.

Then set memResource to NULL here and ...

> +
> +    if (hypervInvokeMethod(priv, params, NULL) < 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Could not set memory"));
> +        goto cleanup;
> +    }
> +
> +    result = 0;
> + cleanup:
> +    VIR_FREE(memory_str);
> +    hypervFreeObject(priv, (hypervObject *) vssd);
> +    hypervFreeObject(priv, (hypervObject *) memsd);

... call the new hypervFreeEmbeddedParam(memResource) here to catch
all the cases that goto here before memResource was added successfully
to the hypervInvokeParamsListPtr. The potential double free is avoided
by setting memResource to NULL after is was successfully added to the
hypervInvokeParamsListPtr that will then take care of its cleanup.

> +    return result;
> +}
> +
> +
> +static int
> +hypervDomainSetMemory(virDomainPtr domain, unsigned long memory)
> +{
> +    return hypervDomainSetMemoryFlags(domain, memory, 0);
> +}
> +
> +
>  static virHypervisorDriver hypervHypervisorDriver = {
>      .name = "Hyper-V",
>      .connectOpen = hypervConnectOpen, /* 0.9.5 */
> @@ -1515,6 +1607,8 @@ static virHypervisorDriver hypervHypervisorDriver = {
>      .domainHasManagedSaveImage = hypervDomainHasManagedSaveImage, /* 0.9.5 */
>      .domainManagedSaveRemove = hypervDomainManagedSaveRemove, /* 0.9.5 */
>      .domainSendKey = hypervDomainSendKey, /* TODO: version */
> +    .domainSetMemory = hypervDomainSetMemory, /* TODO: version */
> +    .domainSetMemoryFlags = hypervDomainSetMemoryFlags, /* TODO: version */
>      .connectIsAlive = hypervConnectIsAlive, /* 0.9.8 */
>  };
>
> diff --git a/src/hyperv/hyperv_wmi.c b/src/hyperv/hyperv_wmi.c
> index 2165838..f50a58c 100644
> --- a/src/hyperv/hyperv_wmi.c
> +++ b/src/hyperv/hyperv_wmi.c
> @@ -1619,3 +1619,54 @@ hypervMsvmComputerSystemFromDomain(virDomainPtr domain,
>
>      return 0;
>  }
> +
> +
> +/* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *
> + * Msvm_VirtualSystemSettingData
> + */
> +
> +int
> +hypervGetMsvmVirtualSystemSettingDataFromUUID(hypervPrivate *priv,
> +        const char *uuid_string, Msvm_VirtualSystemSettingData **list)
> +{
> +    virBuffer query = VIR_BUFFER_INITIALIZER;
> +
> +    virBufferAsprintf(&query,
> +            "associators of "
> +            "{Msvm_ComputerSystem.CreationClassName=\"Msvm_ComputerSystem\","
> +            "Name=\"%s\"} "
> +            "where AssocClass = Msvm_SettingsDefineState "
> +            "ResultClass = Msvm_VirtualSystemSettingData",
> +            uuid_string);
> +
> +    if (hypervGetWmiClassList(priv, Msvm_VirtualSystemSettingData_WmiInfo, &query,
> +                (hypervObject **) list) < 0 || *list == NULL)
> +        return -1;
> +
> +    return 0;
> +}
> +
> +
> +/* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *
> + * Msvm_MemorySettingData
> + */
> +
> +int
> +hypervGetMsvmMemorySettingDataFromVSSD(hypervPrivate *priv,
> +        const char *vssd_instanceid, Msvm_MemorySettingData **list)
> +{
> +    virBuffer query = VIR_BUFFER_INITIALIZER;
> +
> +    virBufferAsprintf(&query,
> +            "associators of "
> +            "{Msvm_VirtualSystemSettingData.InstanceID=\"%s\"} "
> +            "where AssocClass = Msvm_VirtualSystemSettingDataComponent "
> +            "ResultClass = Msvm_MemorySettingData",
> +            vssd_instanceid);
> +
> +    if (hypervGetWmiClassList(priv, Msvm_MemorySettingData_WmiInfo, &query,
> +                (hypervObject **) list) < 0 || *list == NULL)
> +        return -1;
> +
> +    return 0;
> +}
> diff --git a/src/hyperv/hyperv_wmi.h b/src/hyperv/hyperv_wmi.h
> index eb6f43d..c979b99 100644
> --- a/src/hyperv/hyperv_wmi.h
> +++ b/src/hyperv/hyperv_wmi.h
> @@ -35,6 +35,9 @@
>
>  # define HYPERV_DEFAULT_PARAM_COUNT 5
>
> +# define MSVM_VIRTUALSYSTEMMANAGEMENTSERVICE_SELECTOR \
> +    "CreationClassName=Msvm_VirtualSystemManagementService"
> +
>  int hypervVerifyResponse(WsManClient *client, WsXmlDocH response,
>                           const char *detail);
>
> @@ -210,6 +213,10 @@ int hypervGetMsvmVirtualSystemSettingDataList(hypervPrivate *priv,
>                                                virBufferPtr query,
>                                                Msvm_VirtualSystemSettingData **list);
>
> +int hypervGetMsvmVirtualSystemSettingDataFromUUID(hypervPrivate *priv,
> +                                                  const char *uuid_string,
> +                                                  Msvm_VirtualSystemSettingData **list);
> +
>  int hypervGetMsvmProcessorSettingDataList(hypervPrivate *priv,
>                                            virBufferPtr query,
>                                            Msvm_ProcessorSettingData **list);
> @@ -217,6 +224,10 @@ int hypervGetMsvmProcessorSettingDataList(hypervPrivate *priv,
>  int hypervGetMsvmMemorySettingDataList(hypervPrivate *priv, virBufferPtr query,
>                                         Msvm_MemorySettingData **list);
>
> +int hypervGetMsvmMemorySettingDataFromVSSD(hypervPrivate *priv,
> +                                           const char *vssd_instanceid,
> +                                           Msvm_MemorySettingData **list);
> +
>  int hypervGetMsvmKeyboardList(hypervPrivate *priv, virBufferPtr query,
>                                         Msvm_Keyboard **list);
>
> diff --git a/src/hyperv/hyperv_wmi_generator.input b/src/hyperv/hyperv_wmi_generator.input
> index 4ccda04..da19765 100644
> --- a/src/hyperv/hyperv_wmi_generator.input
> +++ b/src/hyperv/hyperv_wmi_generator.input
> @@ -787,6 +787,36 @@ class Msvm_VirtualSystemManagementService
>      boolean  Started
>  end
>
> +class v2/Msvm_VirtualSystemManagementService
> +    string   InstanceID
> +    string   Caption
> +    string   Description
> +    string   ElementName
> +    datetime InstallDate
> +    string   Name
> +    uint16   OperationalStatus[]
> +    string   StatusDescriptions[]
> +    string   Status
> +    uint16   HealthState
> +    uint16   CommunicationStatus
> +    uint16   DetailedStatus
> +    uint16   OperatingStatus
> +    uint16   PrimaryStatus
> +    uint16   EnabledState
> +    string   OtherEnabledState
> +    uint16   RequestedState
> +    uint16   EnabledDefault
> +    datetime TimeOfLastStateChange
> +    uint16   AvailableRequestedStates[]
> +    uint16   TransitioningToState
> +    string   SystemCreationClassName
> +    string   SystemName
> +    string   CreationClassName
> +    string   PrimaryOwnerName
> +    string   PrimaryOwnerContact
> +    string   StartMode
> +    boolean  Started
> +end
>
>  class Msvm_VirtualSystemGlobalSettingData
>      string   Caption
> --
> 2.9.4
>
> --
> libvir-list mailing list
> libvir-list redhat com
> https://www.redhat.com/mailman/listinfo/libvir-list



-- 
Matthias Bolte
http://photron.blogspot.com


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